Re: [PATCH PROTOTYPE 4/6] memory: Extend ram_block_discard_(require|disable) by two discard types

[Peter Xu]

On Tue, Oct 20, 2020 at 04:49:29PM -0400, Peter Xu wrote:
> On Tue, Oct 20, 2020 at 09:58:34PM +0200, David Hildenbrand wrote:
> > I remember there were some !BQL users (but I might be confusing it with
> > postcopy code that once used to inhibit the balloon without BQL). Will
> > double-check. Simplifying it is certainly a good idea.
> > 
> > (we want to be able to check from virtio-balloon code repeatedly without
> > taking a mutex over and over again :) )
> 
> Right.  Again I've no strong opinion so feel free to keep the codes as wish.
> However if we'd go the other way (either BQL or another mutex) IMHO we can
> simply read that flag directly without taking mutex.  IMHO here the mutex can
> be used to protect write concurrency and should be enough. Considering that
> this flag should rarely change and it should never flip (e.g., positive would
> never go negative, and vise versa), then READ_ONCE() whould be safe enough on
> read side (e.g., balloon).

Btw, what we've discussed is all about serialzation of the flag.  I'm also
thinking about whether we can make the flag clearer on what it means.  Frankly
speaking on the first glance it confused me quite a bit..

IMHO what we may want is not some complicated counter on "disablement", but
some simple counters on "someone that provided the cap to discard pages", and
"someone that won't work if we _might_ discard pages".  I'm thinking what if we
split the "disable" counter into two:

  - ram_discard_providers ("Who allows ram discard"): balloon, virtio-mem

  - ram_discard_opposers ("Who forbids ram discard"): vfio, rdma, ...

The major benefit is, the counters should always be >=0, as what a usual
counter would do.  Each device/caller should only/majorly increase the counter.
Also we don't need the cmpxchg() loops too since the check is easier - just
read the other side of the counters to know whether we should fail now.

So after this patch to introduce "coordinated discards", the counters can also
grows into four (we can still define some arrays):

        |---------------------------+------------+-----------|
        | counters                  | provider   | opposer   |
        |---------------------------+------------+-----------|
        | RAM_DISCARD_COORDINATED   | virtio-mem | rdma, ... |
        | RAM_DISCARD_UNCOORDINATED | balloon    | vfio      |
        |---------------------------+------------+-----------|

Some examples: for vfio, it only needs to make sure no UNCOORDINATE providers.
For rdma, it needs to make sure no COORDINATE/UNCOORDINATE providers.  The
check helper could simply be done using a similar ANY bitmask as introduced in
the current patch.

Not sure whether this may help.

Thanks,

-- 
Peter Xu