[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 18.12.19 15:40, SeongJae Park wrote:
On Wed, 18 Dec 2019 14:30:44 +0100 "Jürgen Groß" <jgross@xxxxxxxx> wrote:

On 18.12.19 13:42, SeongJae Park wrote:
On Wed, 18 Dec 2019 13:27:37 +0100 "Jürgen Groß" <jgross@xxxxxxxx> wrote:

On 18.12.19 11:42, 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.

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.

I'm still worrying about the unnecessarily large critical section, but it might
be small enough to be ignored.  If no others have strong objection, I will take
the lock around the '->probe()' and '->remove()'.

The lock is per device, so contention is possible only for the
reclaim case. In case probe or remove are running reclaim will have
nothing to free (in probe case nothing is allocated yet, in remove
case everything should be freed anyway). So the larger critical section
is no problem at all IMO.

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.

Yes, you're right.  I will try test with lockdep.

Thanks,


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.