| Message ID | 20220201162508.417008-3-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough pre-req patches | expand |
Hi, On 01/02/2022 16:25, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > This helper destroys all the ranges of the rangeset given. > Please note, that it uses rangeset_remove_range which returns an error > code on failure. This error code can be ignored as while destroying all > the ranges no memory allocation is expected, so in this case it must not > fail. > > To make sure this remains valid use BUG_ON if that changes in the future. > > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > xen/common/rangeset.c | 6 ++++++ > xen/include/xen/rangeset.h | 3 +++ > 2 files changed, 9 insertions(+) > > diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c > index ea27d651723b..9ca2b06cff22 100644 > --- a/xen/common/rangeset.c > +++ b/xen/common/rangeset.c > @@ -525,6 +525,12 @@ void rangeset_swap(struct rangeset *a, struct rangeset *b) > write_unlock(&b->lock); > } > > +void rangeset_reset(struct rangeset *r) > +{ > + /* This doesn't allocate anything and must not fail. */ > + BUG_ON(rangeset_remove_range(r, 0, ~0ULL)); I vaguely recall some discussion in the past (not related to this series) that we wanted to avoid calling function have side-effect in a BUG_ON(). So if we decide to remove at compile-time BUG_ON(), there would be no issue. But then I am not sure about the use of BUG_ON(). Can you outline why crashing the hypervisor is better than continuing (e.g. use WARN_ON() or ASSERT())? Cheers,
Hi, Julien! On 01.02.22 19:05, Julien Grall wrote: > Hi, > > On 01/02/2022 16:25, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> This helper destroys all the ranges of the rangeset given. >> Please note, that it uses rangeset_remove_range which returns an error >> code on failure. This error code can be ignored as while destroying all >> the ranges no memory allocation is expected, so in this case it must not >> fail. >> >> To make sure this remains valid use BUG_ON if that changes in the future. >> >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> xen/common/rangeset.c | 6 ++++++ >> xen/include/xen/rangeset.h | 3 +++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c >> index ea27d651723b..9ca2b06cff22 100644 >> --- a/xen/common/rangeset.c >> +++ b/xen/common/rangeset.c >> @@ -525,6 +525,12 @@ void rangeset_swap(struct rangeset *a, struct rangeset *b) >> write_unlock(&b->lock); >> } >> +void rangeset_reset(struct rangeset *r) >> +{ >> + /* This doesn't allocate anything and must not fail. */ >> + BUG_ON(rangeset_remove_range(r, 0, ~0ULL)); > > I vaguely recall some discussion in the past (not related to this series) that we wanted to avoid calling function have side-effect in a BUG_ON(). So if we decide to remove at compile-time BUG_ON(), there would be no issue. > > But then I am not sure about the use of BUG_ON(). Can you outline why crashing the hypervisor is better than continuing (e.g. use WARN_ON() or ASSERT())? Non-zero value will indicate we were not able to complete the operation which must not fail because of the concrete use-case: we remove all the ranges and it is not expected that this may fail. Just to make sure this behavior does not change I use BUG_ON here which if triggered clearly indicates that the behavior has changed and there is a need in code change. I can turn this into WARN_ON instead, but this may lead to memory leaks or some other errors not handled. > > Cheers, > Thank you, Oleksandr
On 01/02/2022 17:14, Oleksandr Andrushchenko wrote: > On 01.02.22 19:05, Julien Grall wrote: >> On 01/02/2022 16:25, Oleksandr Andrushchenko wrote: >>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> >>> This helper destroys all the ranges of the rangeset given. >>> Please note, that it uses rangeset_remove_range which returns an error >>> code on failure. This error code can be ignored as while destroying all >>> the ranges no memory allocation is expected, so in this case it must not >>> fail. >>> >>> To make sure this remains valid use BUG_ON if that changes in the future. >>> >>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> --- >>> xen/common/rangeset.c | 6 ++++++ >>> xen/include/xen/rangeset.h | 3 +++ >>> 2 files changed, 9 insertions(+) >>> >>> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c >>> index ea27d651723b..9ca2b06cff22 100644 >>> --- a/xen/common/rangeset.c >>> +++ b/xen/common/rangeset.c >>> @@ -525,6 +525,12 @@ void rangeset_swap(struct rangeset *a, struct rangeset *b) >>> write_unlock(&b->lock); >>> } >>> +void rangeset_reset(struct rangeset *r) >>> +{ >>> + /* This doesn't allocate anything and must not fail. */ >>> + BUG_ON(rangeset_remove_range(r, 0, ~0ULL)); >> >> I vaguely recall some discussion in the past (not related to this series) that we wanted to avoid calling function have side-effect in a BUG_ON(). So if we decide to remove at compile-time BUG_ON(), there would be no issue. >> >> But then I am not sure about the use of BUG_ON(). Can you outline why crashing the hypervisor is better than continuing (e.g. use WARN_ON() or ASSERT())? > Non-zero value will indicate we were not able to complete the operation > which must not fail because of the concrete use-case: we remove all the > ranges and it is not expected that this may fail. > Just to make sure this behavior does not change I use BUG_ON here which > if triggered clearly indicates that the behavior has changed and there is > a need in code change. Right, but that change of behavior may not be noticed during development. So I think we want to avoid BUG_ON() when this is possible. > > I can turn this into WARN_ON instead, but this may lead to memory leaks > or some other errors not handled. IMHO, this is a bit better but not by much. Looking a rangeset_destroy(), you should be able to do it without any of the issues you described here. Something like: if ( r == NULL ) return; while ( (x = first_range(r)) != NULL ) destroy_range(r, x);
On 01.02.22 19:33, Julien Grall wrote: > > > On 01/02/2022 17:14, Oleksandr Andrushchenko wrote: >> On 01.02.22 19:05, Julien Grall wrote: >>> On 01/02/2022 16:25, Oleksandr Andrushchenko wrote: >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> >>>> This helper destroys all the ranges of the rangeset given. >>>> Please note, that it uses rangeset_remove_range which returns an error >>>> code on failure. This error code can be ignored as while destroying all >>>> the ranges no memory allocation is expected, so in this case it must not >>>> fail. >>>> >>>> To make sure this remains valid use BUG_ON if that changes in the future. >>>> >>>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> --- >>>> xen/common/rangeset.c | 6 ++++++ >>>> xen/include/xen/rangeset.h | 3 +++ >>>> 2 files changed, 9 insertions(+) >>>> >>>> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c >>>> index ea27d651723b..9ca2b06cff22 100644 >>>> --- a/xen/common/rangeset.c >>>> +++ b/xen/common/rangeset.c >>>> @@ -525,6 +525,12 @@ void rangeset_swap(struct rangeset *a, struct rangeset *b) >>>> write_unlock(&b->lock); >>>> } >>>> +void rangeset_reset(struct rangeset *r) >>>> +{ >>>> + /* This doesn't allocate anything and must not fail. */ >>>> + BUG_ON(rangeset_remove_range(r, 0, ~0ULL)); >>> >>> I vaguely recall some discussion in the past (not related to this series) that we wanted to avoid calling function have side-effect in a BUG_ON(). So if we decide to remove at compile-time BUG_ON(), there would be no issue. >>> >>> But then I am not sure about the use of BUG_ON(). Can you outline why crashing the hypervisor is better than continuing (e.g. use WARN_ON() or ASSERT())? >> Non-zero value will indicate we were not able to complete the operation >> which must not fail because of the concrete use-case: we remove all the >> ranges and it is not expected that this may fail. >> Just to make sure this behavior does not change I use BUG_ON here which >> if triggered clearly indicates that the behavior has changed and there is >> a need in code change. > > Right, but that change of behavior may not be noticed during development. So I think we want to avoid BUG_ON() when this is possible. > >> >> I can turn this into WARN_ON instead, but this may lead to memory leaks >> or some other errors not handled. > > IMHO, this is a bit better but not by much. Looking a rangeset_destroy(), you should be able to do it without any of the issues you described here. Something like: > > if ( r == NULL ) > return; > > while ( (x = first_range(r)) != NULL ) > destroy_range(r, x); > Yes, this is actually what Roger suggested to me privately on IRC. Ok, so I'll re-work the patch as above Thank you, Oleksandr
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c index ea27d651723b..9ca2b06cff22 100644 --- a/xen/common/rangeset.c +++ b/xen/common/rangeset.c @@ -525,6 +525,12 @@ void rangeset_swap(struct rangeset *a, struct rangeset *b) write_unlock(&b->lock); } +void rangeset_reset(struct rangeset *r) +{ + /* This doesn't allocate anything and must not fail. */ + BUG_ON(rangeset_remove_range(r, 0, ~0ULL)); +} + /***************************** * Pretty-printing functions */ diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h index f7c69394d66a..e0d70d88bdd7 100644 --- a/xen/include/xen/rangeset.h +++ b/xen/include/xen/rangeset.h @@ -95,6 +95,9 @@ bool_t __must_check rangeset_contains_singleton( /* swap contents */ void rangeset_swap(struct rangeset *a, struct rangeset *b); +/* Destroy all ranges. */ +void rangeset_reset(struct rangeset *r); + /* Rangeset pretty printing. */ void rangeset_domain_printk( struct domain *d);