| Message ID | 20210222115708.7623-10-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-mem: vfio support | expand |
On 22/02/21 12:57, David Hildenbrand wrote: > > > -/* > - * If positive, discarding RAM is disabled. If negative, discarding RAM is > - * required to work and cannot be disabled. > - */ > -static int ram_block_discard_disabled; > +static unsigned int ram_block_discard_requirers; > +static unsigned int ram_block_discard_disablers; Requirer is not an English word, so perhaps use required_cnt and disabled_cnt? 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? Paolo
On 22.02.21 14:14, Paolo Bonzini wrote: > On 22/02/21 12:57, David Hildenbrand wrote: >> >> >> -/* >> - * If positive, discarding RAM is disabled. If negative, discarding RAM is >> - * required to work and cannot be disabled. >> - */ >> -static int ram_block_discard_disabled; >> +static unsigned int ram_block_discard_requirers; >> +static unsigned int ram_block_discard_disablers; > > Requirer is not an English word, so perhaps use required_cnt and > disabled_cnt? I did a internet search back then and was not completely sure if it's okay. See https://en.wiktionary.org/wiki/requirer for example (not 100% trustworthy of course). No strong opinion on the name (required_cnt/disabled_cnt is clearer). > > 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)
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, }; 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); Paolo
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" ? > 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.
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.
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.
On 23/02/21 10:02, David Hildenbrand wrote: > > > 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); > Fair enough. Paolo
diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 19e0aa9836..6550217c26 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -3677,56 +3677,64 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root) } } -/* - * If positive, discarding RAM is disabled. If negative, discarding RAM is - * required to work and cannot be disabled. - */ -static int ram_block_discard_disabled; +static unsigned int ram_block_discard_requirers; +static unsigned int ram_block_discard_disablers; +static QemuMutex ram_block_discard_disable_mutex; + +static void ram_block_discard_disable_mutex_lock(void) +{ + static gsize initialized; + + if (g_once_init_enter(&initialized)) { + qemu_mutex_init(&ram_block_discard_disable_mutex); + g_once_init_leave(&initialized, 1); + } + qemu_mutex_lock(&ram_block_discard_disable_mutex); +} + +static void ram_block_discard_disable_mutex_unlock(void) +{ + qemu_mutex_unlock(&ram_block_discard_disable_mutex); +} int ram_block_discard_disable(bool state) { - int old; + int ret = 0; + ram_block_discard_disable_mutex_lock(); if (!state) { - qatomic_dec(&ram_block_discard_disabled); - return 0; + ram_block_discard_disablers--; + } else if (!ram_block_discard_requirers) { + ram_block_discard_disablers++; + } else { + ret = -EBUSY; } - - do { - old = qatomic_read(&ram_block_discard_disabled); - if (old < 0) { - return -EBUSY; - } - } while (qatomic_cmpxchg(&ram_block_discard_disabled, - old, old + 1) != old); - return 0; + ram_block_discard_disable_mutex_unlock(); + return ret; } int ram_block_discard_require(bool state) { - int old; + int ret = 0; + ram_block_discard_disable_mutex_lock(); if (!state) { - qatomic_inc(&ram_block_discard_disabled); - return 0; + ram_block_discard_requirers--; + } else if (!ram_block_discard_disablers) { + ram_block_discard_requirers++; + } else { + ret = -EBUSY; } - - do { - old = qatomic_read(&ram_block_discard_disabled); - if (old > 0) { - return -EBUSY; - } - } while (qatomic_cmpxchg(&ram_block_discard_disabled, - old, old - 1) != old); - return 0; + ram_block_discard_disable_mutex_unlock(); + return ret; } bool ram_block_discard_is_disabled(void) { - return qatomic_read(&ram_block_discard_disabled) > 0; + return qatomic_read(&ram_block_discard_disablers); } bool ram_block_discard_is_required(void) { - return qatomic_read(&ram_block_discard_disabled) < 0; + return qatomic_read(&ram_block_discard_requirers); }
We have users in migration context that don't hold the BQL (when finishing migration). To prepare for further changes, use a dedicated mutex instead of atomic operations. Keep using qatomic_read ("READ_ONCE") for the functions that only extract the current state (e.g., used by virtio-balloon), locking isn't necessary. While at it, split up the counter into two variables to make it easier to understand. Suggested-by: Peter Xu <peterx@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> Cc: Peter Xu <peterx@redhat.com> Cc: Auger Eric <eric.auger@redhat.com> Cc: Wei Yang <richard.weiyang@linux.alibaba.com> Cc: teawater <teawaterz@linux.alibaba.com> Cc: Marek Kedzierski <mkedzier@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- softmmu/physmem.c | 70 ++++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 31 deletions(-)