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

[David Hildenbrand <david@redhat.com>]

On 22.02.21 18:32, Paolo Bonzini wrote:
> 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);
>>

So, the new API I propose is:

int ram_block_discard_disable(bool state)
int ram_block_uncoordinated_discard_disable(bool state)
int ram_block_discard_require(bool state)
int ram_block_coordinated_discard_require(bool state);
bool ram_block_discard_is_disabled(void);
bool ram_block_discard_is_required(void);


Some points (because I thought about this API a bit when I came up with it):

1. I'd really like to keep the functionality of 
ram_block_discard_is_disabled() / ram_block_discard_is_required(). I'd 
assume you just didn't include it in your proposal.

2. I prefer the "require" wording over "start/finish". Start/finish 
sounds like it's a temporary thing like a transaction. For example 
"ram_block_discard_is_started()" sounds misleading to me

3. "ram_discard_enable()" sounds a bit misleading to me as well. We're 
not actually enabling anything, we're not disabling it anymore.

4. I don't think returning an "Error **errp" does make a lot of sense here.


Unless there is real need for a major overhaul I'd like to keep it to 
minor changes.

-- 
Thanks,

David / dhildenb