Re: Re: [Xen-devel] [PATCH v11 2/6] xenbus/backend: Protect xenbus callback with lock

[SeongJae Park]

On Tue, 17 Dec 2019 17:13:42 +0100 "JÃrgen GroÃ" <jgross@xxxxxxxx> wrote:

> On 17.12.19 17:07, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@xxxxxxxxx>
> > 
> > '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.
> > 
> > Signed-off-by: SeongJae Park <sjpark@xxxxxxxxx>
> > ---
> >   drivers/xen/xenbus/xenbus_probe.c         |  1 +
> >   drivers/xen/xenbus/xenbus_probe_backend.c | 10 ++++++++--
> >   include/xen/xenbus.h                      |  2 ++
> >   3 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > 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..516aa64b9967 100644
> > --- a/drivers/xen/xenbus/xenbus_probe_backend.c
> > +++ b/drivers/xen/xenbus/xenbus_probe_backend.c
> > @@ -251,12 +251,18 @@ 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);
> > +		spin_trylock_irqsave(&xdev->reclaim_lock, flags);
> 
> You need spin_lock_irqsave() here. Or maybe spin_lock() would be fine,
> too? I can't see a reason why you'd want to disable irqs here.

I needed to diable irq here as this is called from the memory shrinker context.

Also, used 'trylock' because the 'probe()' and 'remove()' code of the driver
might include memory allocation.  And the xen-blkback actually does.  If the
allocation shows a memory pressure during the allocation, it will trigger this
shrinker callback again and then deadlock.


Thanks,
SeongJae Park

> 
> > +		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)
> > 
> 
> 
> Juergen
>