| Message ID | 20191218104232.9606-3-sjpark@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xenbus/backend: Add memory pressure handler callback | expand |
On 18.12.19 11:42, SeongJae Park wrote: > From: SeongJae Park <sjpark@amazon.de> > > 'reclaim_memory' callback can race with a driver code as this callback > will be called from any memory pressure detected context. To deal with > the case, this commit adds a spinlock in the 'xenbus_device'. Whenever > 'reclaim_memory' callback is called, the lock of the device which passed > to the callback as its argument is locked. Thus, drivers registering > their 'reclaim_memory' callback should protect the data that might race > with the callback with the lock by themselves. Any reason you don't take the lock around the .probe() and .remove() calls of the backend (xenbus_dev_probe() and xenbus_dev_remove())? This would eliminate the need to do that in each backend instead. Juergen
On Wed, 18 Dec 2019 13:27:37 +0100 "Jürgen Groß" <jgross@suse.com> wrote: > On 18.12.19 11:42, SeongJae Park wrote: > > From: SeongJae Park <sjpark@amazon.de> > > > > 'reclaim_memory' callback can race with a driver code as this callback > > will be called from any memory pressure detected context. To deal with > > the case, this commit adds a spinlock in the 'xenbus_device'. Whenever > > 'reclaim_memory' callback is called, the lock of the device which passed > > to the callback as its argument is locked. Thus, drivers registering > > their 'reclaim_memory' callback should protect the data that might race > > with the callback with the lock by themselves. > > Any reason you don't take the lock around the .probe() and .remove() > calls of the backend (xenbus_dev_probe() and xenbus_dev_remove())? This > would eliminate the need to do that in each backend instead. First of all, I would like to keep the critical section as small as possible. With my small test, I could see slightly increasing memory pressure as the critical section becomes wider. Also, some drivers might share the data their 'reclaim_memory' callback touches with other functions. I think only the driver owners can know what data is shared and what is the minimum critical section to protect it. If you think differently or I am missing something, please let me know. Thanks, SeongJae Park > > > Juergen
On 18.12.19 13:42, SeongJae Park wrote: > On Wed, 18 Dec 2019 13:27:37 +0100 "Jürgen Groß" <jgross@suse.com> wrote: > >> On 18.12.19 11:42, SeongJae Park wrote: >>> From: SeongJae Park <sjpark@amazon.de> >>> >>> 'reclaim_memory' callback can race with a driver code as this callback >>> will be called from any memory pressure detected context. To deal with >>> the case, this commit adds a spinlock in the 'xenbus_device'. Whenever >>> 'reclaim_memory' callback is called, the lock of the device which passed >>> to the callback as its argument is locked. Thus, drivers registering >>> their 'reclaim_memory' callback should protect the data that might race >>> with the callback with the lock by themselves. >> >> Any reason you don't take the lock around the .probe() and .remove() >> calls of the backend (xenbus_dev_probe() and xenbus_dev_remove())? This >> would eliminate the need to do that in each backend instead. > > First of all, I would like to keep the critical section as small as possible. > With my small test, I could see slightly increasing memory pressure as the > critical section becomes wider. Also, some drivers might share the data their > 'reclaim_memory' callback touches with other functions. I think only the > driver owners can know what data is shared and what is the minimum critical > section to protect it. But this kind of serialization can still be added on top. And with the trylock in the reclaim path I believe you can even avoid the irq variants of the spinlock. But I might be wrong, so you should try that with lockdep enabled. If it is working there is no harm done when making the critical section larger, as memory allocations will work as before. Juergen
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index 5b471889d723..b86393f172e6 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -472,6 +472,7 @@ int xenbus_probe_node(struct xen_bus_type *bus, goto fail; dev_set_name(&xendev->dev, "%s", devname); + spin_lock_init(&xendev->reclaim_lock); /* Register with generic device framework. */ err = device_register(&xendev->dev); diff --git a/drivers/xen/xenbus/xenbus_probe_backend.c b/drivers/xen/xenbus/xenbus_probe_backend.c index 7e78ebef7c54..e862cb932cc4 100644 --- a/drivers/xen/xenbus/xenbus_probe_backend.c +++ b/drivers/xen/xenbus/xenbus_probe_backend.c @@ -251,12 +251,19 @@ static int backend_probe_and_watch(struct notifier_block *notifier, static int backend_reclaim_memory(struct device *dev, void *data) { const struct xenbus_driver *drv; + struct xenbus_device *xdev; + unsigned long flags; if (!dev->driver) return 0; drv = to_xenbus_driver(dev->driver); - if (drv && drv->reclaim_memory) - drv->reclaim_memory(to_xenbus_device(dev)); + if (drv && drv->reclaim_memory) { + xdev = to_xenbus_device(dev); + if (!spin_trylock_irqsave(&xdev->reclaim_lock, flags)) + return 0; + drv->reclaim_memory(xdev); + spin_unlock_irqrestore(&xdev->reclaim_lock, flags); + } return 0; } diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index c861cfb6f720..d9468313061d 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -76,6 +76,8 @@ struct xenbus_device { enum xenbus_state state; struct completion down; struct work_struct work; + /* 'reclaim_memory' callback is called while this lock is acquired */ + spinlock_t reclaim_lock; }; static inline struct xenbus_device *to_xenbus_device(struct device *dev)