|
From: | David Hildenbrand |
Subject: | Re: [PATCH v6 09/12] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require) |
Date: | Tue, 23 Feb 2021 10:02:18 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
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... Paolobool 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
[Prev in Thread] | Current Thread | [Next in Thread] |