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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2351 bytes --]

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 #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]