| Message ID | 20201125105122.3650-2-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: > In order to avoid latent races when updating an event channel put > xen_consumer and pending fields in different bytes. This is no problem > right now, but especially the pending indicator isn't used only when > initializing an event channel (unlike xen_consumer), so any future > addition to this byte would need to be done with a potential race kept > in mind. > > At the same time move some other fields around to have less implicit > paddings and to keep related fields more closely together. > > Finally switch struct evtchn to no longer use fixed sized types where > not needed. > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one more adjustment (can be done while committing, I guess): > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -93,31 +93,33 @@ struct evtchn > #define ECS_PIRQ 4 /* Channel is bound to a physical IRQ line. */ > #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 */ > - u8 pending:1; > - u16 notify_vcpu_id; /* VCPU for local delivery notification */ > - u32 port; > + unsigned char state; /* ECS_* */ > +#ifndef NDEBUG > + unsigned char old_state; /* State when taking lock in write mode. */ > +#endif > + unsigned char xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if != 0 */ > + unsigned int port; evtchn_port_t, to be in line with ... > union { > struct { > domid_t remote_domid; > - } unbound; /* state == ECS_UNBOUND */ > + } unbound; /* state == ECS_UNBOUND */ > struct { > evtchn_port_t remote_port; > struct domain *remote_dom; > - } interdomain; /* state == ECS_INTERDOMAIN */ > + } interdomain; /* state == ECS_INTERDOMAIN */ > struct { > - u32 irq; > + unsigned int irq; > evtchn_port_t next_port; > evtchn_port_t prev_port; ... three of the fields above from here. > - } pirq; /* state == ECS_PIRQ */ > - u16 virq; /* state == ECS_VIRQ */ > + } pirq; /* state == ECS_PIRQ */ > + unsigned int 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. */ > + > + bool pending; /* FIFO event channels only. */ > + unsigned char priority; /* FIFO event channels only. */ > + unsigned short notify_vcpu_id; /* VCPU for local delivery notification */ I have to admit though that I'm not fully happy with the uses of "unsigned char" and "unsigned short". Yes, I did ask for this change (based on ./CODING_STYLE), but I did also hint towards the use of bitfields. If bitfields aren't an option here to achieve the desired dense packing, perhaps this desire should be permitted as another reason to use fixed width types. (Question goes more towards everyone who cares than to you specifically.) Otoh, as long as we have the odd alignment attribute on the struct, packing density doesn't really matter all this much. I was more hoping for this change to be a step towards us dropping that attribute. Jan
Hi Jan, On 27/11/2020 11:42, Jan Beulich wrote: > On 25.11.2020 11:51, Juergen Gross wrote: >> In order to avoid latent races when updating an event channel put >> xen_consumer and pending fields in different bytes. This is no problem >> right now, but especially the pending indicator isn't used only when >> initializing an event channel (unlike xen_consumer), so any future >> addition to this byte would need to be done with a potential race kept >> in mind. >> >> At the same time move some other fields around to have less implicit >> paddings and to keep related fields more closely together. >> >> Finally switch struct evtchn to no longer use fixed sized types where >> not needed. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one more adjustment (can be done while committing, I guess): > >> --- a/xen/include/xen/sched.h >> +++ b/xen/include/xen/sched.h >> @@ -93,31 +93,33 @@ struct evtchn >> #define ECS_PIRQ 4 /* Channel is bound to a physical IRQ line. */ >> #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 */ >> - u8 pending:1; >> - u16 notify_vcpu_id; /* VCPU for local delivery notification */ >> - u32 port; >> + unsigned char state; /* ECS_* */ >> +#ifndef NDEBUG >> + unsigned char old_state; /* State when taking lock in write mode. */ >> +#endif >> + unsigned char xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if != 0 */ >> + unsigned int port; > > evtchn_port_t, to be in line with ... > >> union { >> struct { >> domid_t remote_domid; >> - } unbound; /* state == ECS_UNBOUND */ >> + } unbound; /* state == ECS_UNBOUND */ >> struct { >> evtchn_port_t remote_port; >> struct domain *remote_dom; >> - } interdomain; /* state == ECS_INTERDOMAIN */ >> + } interdomain; /* state == ECS_INTERDOMAIN */ >> struct { >> - u32 irq; >> + unsigned int irq; >> evtchn_port_t next_port; >> evtchn_port_t prev_port; > > ... three of the fields above from here. > >> - } pirq; /* state == ECS_PIRQ */ >> - u16 virq; /* state == ECS_VIRQ */ >> + } pirq; /* state == ECS_PIRQ */ >> + unsigned int 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. */ >> + >> + bool pending; /* FIFO event channels only. */ >> + unsigned char priority; /* FIFO event channels only. */ >> + unsigned short notify_vcpu_id; /* VCPU for local delivery notification */ > > I have to admit though that I'm not fully happy with the uses of > "unsigned char" and "unsigned short". Yes, I did ask for this > change (based on ./CODING_STYLE), but I did also hint towards the > use of bitfields. If bitfields aren't an option here to achieve > the desired dense packing, perhaps this desire should be permitted > as another reason to use fixed width types. (Question goes more > towards everyone who cares than to you specifically.) I think uint*_t would make sense here because they are storing information received from an hypercall (all the fields should be fixed size there). But I am also fine the current patch as it is still readable. Cheers,
On 27.11.2020 12:57, Julien Grall wrote: > On 27/11/2020 11:42, Jan Beulich wrote: >> I have to admit though that I'm not fully happy with the uses of >> "unsigned char" and "unsigned short". Yes, I did ask for this >> change (based on ./CODING_STYLE), but I did also hint towards the >> use of bitfields. If bitfields aren't an option here to achieve >> the desired dense packing, perhaps this desire should be permitted >> as another reason to use fixed width types. (Question goes more >> towards everyone who cares than to you specifically.) > > I think uint*_t would make sense here because they are storing > information received from an hypercall (all the fields should be fixed > size there). "storing information received from a hypercall" is specifically not a reason to use fixed width types, imo. All of uint8_t, uint16_t, and uint32_t values coming from hypercalls are fine to be passed around and stored as unsigned int, just as an example. It is solely the packing aspect which might matter here. > But I am also fine the current patch as it is still readable. Good, thanks for checking. Jan
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c index 79090c04ca..f39e61105f 100644 --- a/xen/common/event_fifo.c +++ b/xen/common/event_fifo.c @@ -200,7 +200,7 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn) */ if ( unlikely(!word) ) { - evtchn->pending = 1; + evtchn->pending = true; return; } @@ -535,7 +535,7 @@ static void setup_ports(struct domain *d, unsigned int prev_evtchns) evtchn = evtchn_from_port(d, port); if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) ) - evtchn->pending = 1; + evtchn->pending = true; evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT); } diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index a345cc01f8..7afbae7dd1 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -93,31 +93,33 @@ struct evtchn #define ECS_PIRQ 4 /* Channel is bound to a physical IRQ line. */ #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 */ - u8 pending:1; - u16 notify_vcpu_id; /* VCPU for local delivery notification */ - u32 port; + unsigned char state; /* ECS_* */ +#ifndef NDEBUG + unsigned char old_state; /* State when taking lock in write mode. */ +#endif + unsigned char xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if != 0 */ + unsigned int port; union { struct { domid_t remote_domid; - } unbound; /* state == ECS_UNBOUND */ + } unbound; /* state == ECS_UNBOUND */ struct { evtchn_port_t remote_port; struct domain *remote_dom; - } interdomain; /* state == ECS_INTERDOMAIN */ + } interdomain; /* state == ECS_INTERDOMAIN */ struct { - u32 irq; + unsigned int irq; evtchn_port_t next_port; evtchn_port_t prev_port; - } pirq; /* state == ECS_PIRQ */ - u16 virq; /* state == ECS_VIRQ */ + } pirq; /* state == ECS_PIRQ */ + unsigned int 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. */ + + bool pending; /* FIFO event channels only. */ + unsigned char priority; /* FIFO event channels only. */ + unsigned short notify_vcpu_id; /* VCPU for local delivery notification */ + uint32_t fifo_lastq; /* Data for identifying last queue. */ + #ifdef CONFIG_XSM union { #ifdef XSM_NEED_GENERIC_EVTCHN_SSID @@ -133,7 +135,7 @@ struct evtchn * allocations, and on 64-bit platforms with only FLASK enabled, * reduces the size of struct evtchn. */ - u32 flask_sid; + uint32_t flask_sid; #endif } ssid; #endif