| Message ID | 571F42C802000078000E5AAF@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 26 April 2016 09:28 > To: xen-devel > Cc: Paul Durrant; Wei Liu > Subject: [PATCH] x86/HVM: slightly improve hvm_mmio_{first,last}_byte() > > EFLAGS.DF can be assumed to be usually clear, so unlikely()-annotate > the conditionals accordingly. > > Also prefer latching p->size (used twice) into a local variable, at Well, it should only be used once since only one of the expressions should be evaluated. > once making it unnecessary for the reader to be sure expressions get > evaluated left to right (operand promotion would yield a different > result if p->addr + p->size - 1 was evaluted right to left). Would that not be cured by replacing 1 with 1ul? Paul > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/include/asm-x86/hvm/io.h > +++ b/xen/include/asm-x86/hvm/io.h > @@ -44,18 +44,18 @@ struct hvm_mmio_ops { > > static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p) > { > - return p->df ? > + return unlikely(p->df) ? > p->addr - (p->count - 1ul) * p->size : > p->addr; > } > > static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p) > { > - unsigned long count = p->count; > + unsigned long size = p->size; > > - return p->df ? > - p->addr + p->size - 1: > - p->addr + (count * p->size) - 1; > + return unlikely(p->df) ? > + p->addr + size - 1: > + p->addr + (p->count * size) - 1; > } > > typedef int (*portio_action_t)( > >
>>> On 26.04.16 at 10:41, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 26 April 2016 09:28 >> To: xen-devel >> Cc: Paul Durrant; Wei Liu >> Subject: [PATCH] x86/HVM: slightly improve hvm_mmio_{first,last}_byte() >> >> EFLAGS.DF can be assumed to be usually clear, so unlikely()-annotate >> the conditionals accordingly. >> >> Also prefer latching p->size (used twice) into a local variable, at > > Well, it should only be used once since only one of the expressions should > be evaluated. But there would still be two references in code, and the trivial line of thought here is that (leaving optimization aside) accessing some structure field twice generate code no smaller (and possibly larger) than accessing some other structure field just once. >> once making it unnecessary for the reader to be sure expressions get >> evaluated left to right (operand promotion would yield a different >> result if p->addr + p->size - 1 was evaluted right to left). > > Would that not be cured by replacing 1 with 1ul? That's another possibility, but (being a matter of taste) I prefer to avoid type suffixes. Jan >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/include/asm-x86/hvm/io.h >> +++ b/xen/include/asm-x86/hvm/io.h >> @@ -44,18 +44,18 @@ struct hvm_mmio_ops { >> >> static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p) >> { >> - return p->df ? >> + return unlikely(p->df) ? >> p->addr - (p->count - 1ul) * p->size : >> p->addr; >> } >> >> static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p) >> { >> - unsigned long count = p->count; >> + unsigned long size = p->size; >> >> - return p->df ? >> - p->addr + p->size - 1: >> - p->addr + (count * p->size) - 1; >> + return unlikely(p->df) ? >> + p->addr + size - 1: >> + p->addr + (p->count * size) - 1; >> } >> >> typedef int (*portio_action_t)( >> >>
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 26 April 2016 09:47 > To: Paul Durrant > Cc: Wei Liu; xen-devel > Subject: RE: [PATCH] x86/HVM: slightly improve > hvm_mmio_{first,last}_byte() > > >>> On 26.04.16 at 10:41, <Paul.Durrant@citrix.com> wrote: > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: 26 April 2016 09:28 > >> To: xen-devel > >> Cc: Paul Durrant; Wei Liu > >> Subject: [PATCH] x86/HVM: slightly improve > hvm_mmio_{first,last}_byte() > >> > >> EFLAGS.DF can be assumed to be usually clear, so unlikely()-annotate > >> the conditionals accordingly. > >> > >> Also prefer latching p->size (used twice) into a local variable, at > > > > Well, it should only be used once since only one of the expressions should > > be evaluated. > > But there would still be two references in code, and the trivial line > of thought here is that (leaving optimization aside) accessing some > structure field twice generate code no smaller (and possibly larger) > than accessing some other structure field just once. > > >> once making it unnecessary for the reader to be sure expressions get > >> evaluated left to right (operand promotion would yield a different > >> result if p->addr + p->size - 1 was evaluted right to left). > > > > Would that not be cured by replacing 1 with 1ul? > > That's another possibility, but (being a matter of taste) I prefer to > avoid type suffixes. > Fair enough, I have no particular preference either way. I guess it would be nice if hvm_mmio_first_byte() and hvm_mmio_last_byte() were consistent in their use of type suffixes though. However... Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > Jan > > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> > >> --- a/xen/include/asm-x86/hvm/io.h > >> +++ b/xen/include/asm-x86/hvm/io.h > >> @@ -44,18 +44,18 @@ struct hvm_mmio_ops { > >> > >> static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p) > >> { > >> - return p->df ? > >> + return unlikely(p->df) ? > >> p->addr - (p->count - 1ul) * p->size : > >> p->addr; > >> } > >> > >> static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p) > >> { > >> - unsigned long count = p->count; > >> + unsigned long size = p->size; > >> > >> - return p->df ? > >> - p->addr + p->size - 1: > >> - p->addr + (count * p->size) - 1; > >> + return unlikely(p->df) ? > >> + p->addr + size - 1: > >> + p->addr + (p->count * size) - 1; > >> } > >> > >> typedef int (*portio_action_t)( > >> > >> > >
On Tue, Apr 26, 2016 at 02:28:24AM -0600, Jan Beulich wrote: > EFLAGS.DF can be assumed to be usually clear, so unlikely()-annotate > the conditionals accordingly. > > Also prefer latching p->size (used twice) into a local variable, at > once making it unnecessary for the reader to be sure expressions get > evaluated left to right (operand promotion would yield a different > result if p->addr + p->size - 1 was evaluted right to left). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Release-acked-by: Wei Liu <wei.liu2@citrix.com>
--- a/xen/include/asm-x86/hvm/io.h +++ b/xen/include/asm-x86/hvm/io.h @@ -44,18 +44,18 @@ struct hvm_mmio_ops { static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p) { - return p->df ? + return unlikely(p->df) ? p->addr - (p->count - 1ul) * p->size : p->addr; } static inline paddr_t hvm_mmio_last_byte(const ioreq_t *p) { - unsigned long count = p->count; + unsigned long size = p->size; - return p->df ? - p->addr + p->size - 1: - p->addr + (count * p->size) - 1; + return unlikely(p->df) ? + p->addr + size - 1: + p->addr + (p->count * size) - 1; } typedef int (*portio_action_t)(