| Message ID | 20201125105122.3650-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/events: further locking adjustments | expand |
On 25.11.2020 11:51, Juergen Gross wrote: > --- a/xen/common/event_fifo.c > +++ b/xen/common/event_fifo.c > @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) > return; > } > > + /* > + * Control block not mapped. The guest must not unmask an > + * event until the control block is initialized, so we can > + * just drop the event. > + */ > + if ( unlikely(!v->evtchn_fifo->control_block) ) > + { > + printk(XENLOG_G_WARNING > + "%pv has no FIFO event channel control block\n", v); > + return; > + } This results in bypassing the setting of PENDING and the possible call to evtchn_check_pollers(). It may in particular be the case that a very special purpose guest uses event channels just for waking up pollers, which - afaict - then doesn't require setting up a control block. To give an example, I could easily see an XTF test avoid that step if indeed it's unnecessary. Jan
On 27.11.20 14:27, Jan Beulich wrote: > On 25.11.2020 11:51, Juergen Gross wrote: >> --- a/xen/common/event_fifo.c >> +++ b/xen/common/event_fifo.c >> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) >> return; >> } >> >> + /* >> + * Control block not mapped. The guest must not unmask an >> + * event until the control block is initialized, so we can >> + * just drop the event. >> + */ >> + if ( unlikely(!v->evtchn_fifo->control_block) ) >> + { >> + printk(XENLOG_G_WARNING >> + "%pv has no FIFO event channel control block\n", v); >> + return; >> + } > > This results in bypassing the setting of PENDING and the possible > call to evtchn_check_pollers(). It may in particular be the case > that a very special purpose guest uses event channels just for > waking up pollers, which - afaict - then doesn't require setting > up a control block. To give an example, I could easily see an XTF > test avoid that step if indeed it's unnecessary. Okay, I can move the test after setting PENDING and do a "goto unlock" instead of returning. Juergen
On 25/11/2020 10:51, Juergen Gross wrote: > evtchn_fifo_set_pending() can be simplified a little bit. The commit message is quite light... For posterity, it would be good to explain why the simplication can be done. In particular, there is a chance in behavior after this patch. > Suggested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V8: > - new patch > --- > xen/common/event_fifo.c | 34 +++++++++++++++------------------- > 1 file changed, 15 insertions(+), 19 deletions(-) > > diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c > index 443593c3b3..77609539b1 100644 > --- a/xen/common/event_fifo.c > +++ b/xen/common/event_fifo.c > @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) > return; > } > > + /* > + * Control block not mapped. The guest must not unmask an > + * event until the control block is initialized, so we can > + * just drop the event. > + */ > + if ( unlikely(!v->evtchn_fifo->control_block) ) Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set concurrently to this access. Thankfully, once the control block is mapped, it can't be unmapped. However, there is still a possibility that you may see half of the update. Shouldn't the field access with ACCESS_ONCE()? Cheers,
On 27.11.20 15:23, Julien Grall wrote: > > > On 25/11/2020 10:51, Juergen Gross wrote: >> evtchn_fifo_set_pending() can be simplified a little bit. > > The commit message is quite light... For posterity, it would be good to > explain why the simplication can be done. In particular, there is a > chance in behavior after this patch. > >> Suggested-by: Jan Beulich <jbeulich@suse.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V8: >> - new patch >> --- >> xen/common/event_fifo.c | 34 +++++++++++++++------------------- >> 1 file changed, 15 insertions(+), 19 deletions(-) >> >> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c >> index 443593c3b3..77609539b1 100644 >> --- a/xen/common/event_fifo.c >> +++ b/xen/common/event_fifo.c >> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu >> *v, struct evtchn *evtchn) >> return; >> } >> + /* >> + * Control block not mapped. The guest must not unmask an >> + * event until the control block is initialized, so we can >> + * just drop the event. >> + */ >> + if ( unlikely(!v->evtchn_fifo->control_block) ) > > Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set > concurrently to this access. > > Thankfully, once the control block is mapped, it can't be unmapped. > However, there is still a possibility that you may see half of the update. > > Shouldn't the field access with ACCESS_ONCE()? Shouldn't this be another patch? Especially as the writing side needs the same treatment. Juergen
On 27.11.2020 15:39, Jürgen Groß wrote: > On 27.11.20 15:23, Julien Grall wrote: >> On 25/11/2020 10:51, Juergen Gross wrote: >>> --- a/xen/common/event_fifo.c >>> +++ b/xen/common/event_fifo.c >>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu >>> *v, struct evtchn *evtchn) >>> return; >>> } >>> + /* >>> + * Control block not mapped. The guest must not unmask an >>> + * event until the control block is initialized, so we can >>> + * just drop the event. >>> + */ >>> + if ( unlikely(!v->evtchn_fifo->control_block) ) >> >> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set >> concurrently to this access. >> >> Thankfully, once the control block is mapped, it can't be unmapped. >> However, there is still a possibility that you may see half of the update. >> >> Shouldn't the field access with ACCESS_ONCE()? > > Shouldn't this be another patch? Especially as the writing side needs > the same treatment. Indeed. As said on several different occasions - our code base is full of places where we chance torn accesses, if there really was a compiler to let us down on this. This recurring pattern shouldn't lead to unrelated patches getting bloated, unless _all_ affected sites get touched anyway. Jan
On 27/11/2020 14:39, Jürgen Groß wrote: > On 27.11.20 15:23, Julien Grall wrote: >> >> >> On 25/11/2020 10:51, Juergen Gross wrote: >>> evtchn_fifo_set_pending() can be simplified a little bit. >> >> The commit message is quite light... For posterity, it would be good >> to explain why the simplication can be done. In particular, there is a >> chance in behavior after this patch. >> >>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> V8: >>> - new patch >>> --- >>> xen/common/event_fifo.c | 34 +++++++++++++++------------------- >>> 1 file changed, 15 insertions(+), 19 deletions(-) >>> >>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c >>> index 443593c3b3..77609539b1 100644 >>> --- a/xen/common/event_fifo.c >>> +++ b/xen/common/event_fifo.c >>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu >>> *v, struct evtchn *evtchn) >>> return; >>> } >>> + /* >>> + * Control block not mapped. The guest must not unmask an >>> + * event until the control block is initialized, so we can >>> + * just drop the event. >>> + */ >>> + if ( unlikely(!v->evtchn_fifo->control_block) ) >> >> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set >> concurrently to this access. >> >> Thankfully, once the control block is mapped, it can't be unmapped. >> However, there is still a possibility that you may see half of the >> update. >> >> Shouldn't the field access with ACCESS_ONCE()? > > Shouldn't this be another patch? Especially as the writing side needs > the same treatment. Yes it should. Sorry I should have been clearer. I am happy to also wrote the patch if you feel you had enough with event channel :). Cheers,
On 27/11/2020 14:48, Jan Beulich wrote: > On 27.11.2020 15:39, Jürgen Groß wrote: >> On 27.11.20 15:23, Julien Grall wrote: >>> On 25/11/2020 10:51, Juergen Gross wrote: >>>> --- a/xen/common/event_fifo.c >>>> +++ b/xen/common/event_fifo.c >>>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu >>>> *v, struct evtchn *evtchn) >>>> return; >>>> } >>>> + /* >>>> + * Control block not mapped. The guest must not unmask an >>>> + * event until the control block is initialized, so we can >>>> + * just drop the event. >>>> + */ >>>> + if ( unlikely(!v->evtchn_fifo->control_block) ) >>> >>> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set >>> concurrently to this access. >>> >>> Thankfully, once the control block is mapped, it can't be unmapped. >>> However, there is still a possibility that you may see half of the update. >>> >>> Shouldn't the field access with ACCESS_ONCE()? >> >> Shouldn't this be another patch? Especially as the writing side needs >> the same treatment. > > Indeed. As said on several different occasions - our code base is > full of places where we chance torn accesses, if there really was > a compiler to let us down on this. I am quite amazed you that you managed to test all the version of GCC/Clang that were built and confirm this is unlikely to happen :). > This recurring pattern > shouldn't lead to unrelated patches getting bloated, unless _all_ > affected sites get touched anyway. You probably missed the point where I say "sort of unrelated". This wasn't not a suggestion to fix it here (I should have been clearer though) but instead point out issue as I see them. Cheers,
On 27.11.2020 16:17, Julien Grall wrote: > > > On 27/11/2020 14:48, Jan Beulich wrote: >> On 27.11.2020 15:39, Jürgen Groß wrote: >>> On 27.11.20 15:23, Julien Grall wrote: >>>> On 25/11/2020 10:51, Juergen Gross wrote: >>>>> --- a/xen/common/event_fifo.c >>>>> +++ b/xen/common/event_fifo.c >>>>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu >>>>> *v, struct evtchn *evtchn) >>>>> return; >>>>> } >>>>> + /* >>>>> + * Control block not mapped. The guest must not unmask an >>>>> + * event until the control block is initialized, so we can >>>>> + * just drop the event. >>>>> + */ >>>>> + if ( unlikely(!v->evtchn_fifo->control_block) ) >>>> >>>> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set >>>> concurrently to this access. >>>> >>>> Thankfully, once the control block is mapped, it can't be unmapped. >>>> However, there is still a possibility that you may see half of the update. >>>> >>>> Shouldn't the field access with ACCESS_ONCE()? >>> >>> Shouldn't this be another patch? Especially as the writing side needs >>> the same treatment. >> >> Indeed. As said on several different occasions - our code base is >> full of places where we chance torn accesses, if there really was >> a compiler to let us down on this. > > I am quite amazed you that you managed to test all the version of > GCC/Clang that were built and confirm this is unlikely to happen :). It's (obviously) not that I tested all of them, but that I know at least a little bit of how gcc generates code, that I'm unaware of reports to the contrary, and that it would seem odd for a compiler to split accesses when they can be done by one insn. Of course one could build a compiler doing only byte accesses ... >> This recurring pattern >> shouldn't lead to unrelated patches getting bloated, unless _all_ >> affected sites get touched anyway. > > You probably missed the point where I say "sort of unrelated". This > wasn't not a suggestion to fix it here (I should have been clearer > though) but instead point out issue as I see them. Point taken. Jan
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c index 443593c3b3..77609539b1 100644 --- a/xen/common/event_fifo.c +++ b/xen/common/event_fifo.c @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) return; } + /* + * Control block not mapped. The guest must not unmask an + * event until the control block is initialized, so we can + * just drop the event. + */ + if ( unlikely(!v->evtchn_fifo->control_block) ) + { + printk(XENLOG_G_WARNING + "%pv has no FIFO event channel control block\n", v); + return; + } + /* * Lock all queues related to the event channel (in case of a queue change * this might be two). @@ -233,25 +245,8 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) * Link the event if it unmasked and not already linked. */ if ( !guest_test_bit(d, EVTCHN_FIFO_MASKED, word) && - !guest_test_bit(d, EVTCHN_FIFO_LINKED, word) ) + !guest_test_and_set_bit(d, EVTCHN_FIFO_LINKED, word) ) { - event_word_t *tail_word; - - /* - * Control block not mapped. The guest must not unmask an - * event until the control block is initialized, so we can - * just drop the event. - */ - if ( unlikely(!v->evtchn_fifo->control_block) ) - { - printk(XENLOG_G_WARNING - "%pv has no FIFO event channel control block\n", v); - goto unlock; - } - - if ( guest_test_and_set_bit(d, EVTCHN_FIFO_LINKED, word) ) - goto unlock; - /* * If this event was a tail, the old queue is now empty and * its tail must be invalidated to prevent adding an event to @@ -286,6 +281,8 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) linked = false; if ( q->tail ) { + event_word_t *tail_word; + tail_word = evtchn_fifo_word_from_port(d, q->tail); linked = evtchn_fifo_set_link(d, tail_word, port); } @@ -294,7 +291,6 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) q->tail = port; } - unlock: if ( q != old_q ) spin_unlock(&old_q->lock); spin_unlock_irqrestore(&q->lock, flags);
evtchn_fifo_set_pending() can be simplified a little bit. Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- V8: - new patch --- xen/common/event_fifo.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-)