Re: [PATCH v6 09/12] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require)

[Paolo Bonzini]

On 22/02/21 16:38, David Hildenbrand wrote:
> On 22.02.21 15:02, Paolo Bonzini wrote:
> > On 22/02/21 14:33, David Hildenbrand wrote:
> > > > Also, uncoordinated require is unused, and therefore uncoordinated
> > > > disable is also never going to block anything.  Does it make sense to
> > > > keep it in the API?
> > >
> > > Right, "ram_block_discard_require()" is not used yet. I am planning on
> > > using it in virtio-balloon context at some point, but can remove it for
> > > now to simplify.
> > >
> > > ram_block_uncoordinated_discard_disable(), however, will block
> > > virtio-balloon already via ram_block_discard_is_disabled(). (yes,
> > > virtio-balloon is ugly)
> >
> > Oops, I missed that API.
> >
> > Does it make sense to turn the API inside out, with the
> > coordinated/uncoordinated choice as an argument and the start/finish
> > choice in the name?
> >
> > enum {
> >       RAM_DISCARD_ALLOW_COORDINATED = 1,
> > };
>
> Any reason to go with an enum/flags for this case and not "bool 
> allow_coordinated" ?

I find it slightly easier to remember the meaning of true for "bool 
coordinated" than for "bool allow_coordinated".  I don't like the API 
below that much, but having both RAM_DISCARD_ALLOW_COORDINATED for 
disable/enable and RAM_DISCARD_SUPPORT_COORDINATED for start/finish 
would be even uglier...

Paolo

> > bool ram_discard_disable(int flags, Error **errp);
> > void ram_discard_enable(int flags);
> > int ram_discard_start(bool coordinated, Error **errp);
> > void ram_discard_finish(bool coordinated);
>
> Yeah, I tried to avoid boolean flags ;) Don't have a strong opinion. At 
> least we get shorter names.