| Message ID | 20201124070106.26854-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/events: further locking adjustments | expand |
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. > @@ -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.) > - 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. Also for all fields you touch anyway, may I ask that you switch to uint<N>_t or, in the case of "pending", bool? Jan
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
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
On 24.11.20 13:37, Jan Beulich wrote: > 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. Okay. > >>> 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. No problem with that. Juergen
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index a345cc01f8..e6d09aa055 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -80,8 +80,7 @@ extern domid_t hardware_domid; #define EVTCHNS_PER_GROUP (BUCKETS_PER_GROUP * EVTCHNS_PER_BUCKET) #define NR_EVTCHN_GROUPS DIV_ROUND_UP(MAX_NR_EVTCHNS, EVTCHNS_PER_GROUP) -#define XEN_CONSUMER_BITS 3 -#define NR_XEN_CONSUMERS ((1 << XEN_CONSUMER_BITS) - 1) +#define NR_XEN_CONSUMERS 8 struct evtchn { @@ -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 */ - 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 */ + u32 fifo_lastq; /* Data for identifying last queue. */ + #ifdef CONFIG_XSM union { #ifdef XSM_NEED_GENERIC_EVTCHN_SSID
In order to avoid latent races when updating an event channel put xen_consumer and pending fields in different bytes. At the same time move some other fields around to have less implicit paddings and to keep related fields more closely together. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/include/xen/sched.h | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)