Re: [PATCH] xen/events: fix build

[Jürgen Groß <jgross@xxxxxxxx>]

On 11.11.20 08:37, Jan Beulich wrote:
> On 11.11.2020 08:27, Jürgen Groß wrote:
> > On 11.11.20 08:20, Jan Beulich wrote:
> > > On 11.11.2020 06:49, Juergen Gross wrote:
> > > > --- a/xen/common/event_channel.c
> > > > +++ b/xen/common/event_channel.c
> > > > @@ -61,7 +61,9 @@ static inline void evtchn_write_lock(struct evtchn *evtchn)
> > > >    {
> > > >        write_lock(&evtchn->lock);
> > > >    
> > > > +#ifndef NDEBUG
> > > >        evtchn->old_state = evtchn->state;
> > > > +#endif
> > > >    }
> > > >    
> > > >    static inline void evtchn_write_unlock(struct evtchn *evtchn)
> > > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > > > index 7251b3ae3e..95f96e7a33 100644
> > > > --- a/xen/include/xen/sched.h
> > > > +++ b/xen/include/xen/sched.h
> > > > @@ -114,9 +114,7 @@ struct evtchn
> > > >            u16 virq;      /* state == ECS_VIRQ */
> > > >        } u;
> > > >        u8 priority;
> > > > -#ifndef NDEBUG
> > > >        u8 old_state;      /* State when taking lock in write mode. */
> > > > -#endif
> > > >        u8 last_priority;
> > > >        u16 last_vcpu_id;
> > > >    #ifdef CONFIG_XSM
> > > Did you mean just either of the two changes (and then the former),
> > > not both? If so
> > > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > and I'll be happy to drop the other half for committing.
> > The header fix is required for NDEBUG builds, while the other one is
> > removing a write with no accompanied read for NDEBUG builds.
> Oh, that's because of our absurd ASSERT() implementation in the
> NDEBUG case. I stand by my position that the field should not be
> there in the first place for release builds. Even more so with
> the original patch having got re-based ahead of what was patch 1
> of the series, which I did not the least because I want that one
> backported urgently, while I continue to be hesitant altogether
> about the other one.
>
> I guess the course of action is then to put #ifndef NDEBUG
> around the ASSERT() itself, however strange this may look. Or
> introduce an evtchn_old_state() wrapper, perhaps returning
> ECS_RESERVED in the NDEBUG case. I guess it'll be quicker if I
> take your patch and massage it before throwing in, than to have
> you make a v2.
Fine with me.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature