[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 24.11.2020 13:18, Jürgen Groß wrote:
> On 24.11.20 12:42, Jan Beulich wrote:
>> On 24.11.2020 08:01, Juergen Gross wrote:
>>> @@ -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.

I'm not meaning to ask to widen the use of bitfields right now
(unless this helps avoiding holes). But I'd like to not see the
one non-problematic use go away without this really being
necessary.

>> 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?

I didn't dare to suggest you doing so. So no, I wouldn't mind.
However, there's more room then for what some would possibly
call bike shedding: The wider the scope of the conversion you
do the more relevant it'll become that strictly speaking there
ought to be (almost?) no use of fixed width types here, as per
./CODING_STYLE.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.