Re: [PATCH v7 2/3] xen/events: modify struct evtchn layout

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

On 24.11.20 12:42, Jan Beulich wrote:
> On 24.11.2020 08:01, Juergen Gross wrote:
> > In order to avoid latent races when updating an event channel put
> > xen_consumer and pending fields in different bytes.
> I think there's a little more to be said here as to what the
> actual risk is, as the two fields are - afaict - at present
> fine the way they're declared.
Okay.

> > @@ -94,9 +93,10 @@ struct evtchn
> >   #define ECS_VIRQ         5 /* Channel is bound to a virtual IRQ line.        
> > */
> >   #define ECS_IPI          6 /* Channel is bound to a virtual IPI line.        
> > */
> >       u8  state;             /* ECS_* */
> > -    u8  xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if nonzero */
> I see no reason to use a full byte for this one; in fact I
> was considering whether it, state, and old_state couldn't
> share storage (the latest when we run into space issues with
> this struct). (In this context I'm also observing that
> old_state could get away with just 2 bits, i.e. all three
> fields would fit in a single byte.)
I think doing further compression now isn't really helping. It would
just add more padding bytes and result in larger code.

> > -    u8  pending:1;
> > -    u16 notify_vcpu_id;    /* VCPU for local delivery notification */
> > +#ifndef NDEBUG
> > +    u8  old_state;     /* State when taking lock in write mode. */
> > +#endif
> > +    u8  xen_consumer;  /* Consumer in Xen if nonzero */
> >       u32 port;
> >       union {
> >           struct {
> > @@ -113,11 +113,13 @@ struct evtchn
> >           } pirq;        /* state == ECS_PIRQ */
> >           u16 virq;      /* state == ECS_VIRQ */
> >       } u;
> > -    u8 priority;
> > -#ifndef NDEBUG
> > -    u8 old_state;      /* State when taking lock in write mode. */
> > -#endif
> > -    u32 fifo_lastq;    /* Data for fifo events identifying last queue. */
> > +
> > +    /* FIFO event channels only. */
> > +    u8  pending;
> > +    u8  priority;
> > +    u16 notify_vcpu_id;    /* VCPU for local delivery notification */
> This field definitely isn't FIFO-only.
Oh, you are right.

> Also for all fields you touch anyway, may I ask that you switch to
> uint<N>_t or, in the case of "pending", bool?
Fine with me.

Would you object to switching the whole structure in this regard?


Juergen

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature