| Message ID | 5CD2D010020000780022CCCC@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: EOI timer corrections / improvements | expand |
On Wed, May 08, 2019 at 06:48:16AM -0600, Jan Beulich wrote: > action->ack_type is set once before the timer even gets initialized, and > is never changed later. The timer gets activated only for EOI and UNMASK > types. Hence there's no need to have a respective if() in there. Replace > it by an ASSERT(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Just one comment below which I'm not overly fussed about. > > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1103,7 +1103,7 @@ static void set_eoi_ready(void *data); > static void irq_guest_eoi_timer_fn(void *data) > { > struct irq_desc *desc = data; > - unsigned int irq = desc - irq_desc; > + unsigned int i, irq = desc - irq_desc; > irq_guest_action_t *action; > cpumask_t cpu_eoi_map; > > @@ -1114,19 +1114,18 @@ static void irq_guest_eoi_timer_fn(void > > action = (irq_guest_action_t *)desc->action; > > + ASSERT(action->ack_type != ACKTYPE_NONE); > + > if ( !action->in_flight || timer_is_active(&action->eoi_timer) ) > goto out; > > - if ( action->ack_type != ACKTYPE_NONE ) > + for ( i = 0; i < action->nr_guests; i++ ) > { > - unsigned int i; > - for ( i = 0; i < action->nr_guests; i++ ) > - { > - struct domain *d = action->guest[i]; I think you could constify d here. Thanks.
>>> On 16.05.19 at 15:52, <roger.pau@citrix.com> wrote: > On Wed, May 08, 2019 at 06:48:16AM -0600, Jan Beulich wrote: >> action->ack_type is set once before the timer even gets initialized, and >> is never changed later. The timer gets activated only for EOI and UNMASK >> types. Hence there's no need to have a respective if() in there. Replace >> it by an ASSERT(). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> @@ -1114,19 +1114,18 @@ static void irq_guest_eoi_timer_fn(void >> >> action = (irq_guest_action_t *)desc->action; >> >> + ASSERT(action->ack_type != ACKTYPE_NONE); >> + >> if ( !action->in_flight || timer_is_active(&action->eoi_timer) ) >> goto out; >> >> - if ( action->ack_type != ACKTYPE_NONE ) >> + for ( i = 0; i < action->nr_guests; i++ ) >> { >> - unsigned int i; >> - for ( i = 0; i < action->nr_guests; i++ ) >> - { >> - struct domain *d = action->guest[i]; > > I think you could constify d here. Ah yes, this should work. Jan
>>> On 16.05.19 at 15:52, <roger.pau@citrix.com> wrote: > On Wed, May 08, 2019 at 06:48:16AM -0600, Jan Beulich wrote: >> @@ -1114,19 +1114,18 @@ static void irq_guest_eoi_timer_fn(void >> >> action = (irq_guest_action_t *)desc->action; >> >> + ASSERT(action->ack_type != ACKTYPE_NONE); >> + >> if ( !action->in_flight || timer_is_active(&action->eoi_timer) ) >> goto out; >> >> - if ( action->ack_type != ACKTYPE_NONE ) >> + for ( i = 0; i < action->nr_guests; i++ ) >> { >> - unsigned int i; >> - for ( i = 0; i < action->nr_guests; i++ ) >> - { >> - struct domain *d = action->guest[i]; > > I think you could constify d here. Now that I've tried I recall that I did so already when originally putting together the patch. It doesn't work, because radix_tree_lookup() requires a non-const pointer. Jan
On 08/05/2019 13:48, Jan Beulich wrote: > action->ack_type is set once before the timer even gets initialized, and > is never changed later. The timer gets activated only for EOI and UNMASK > types. Hence there's no need to have a respective if() in there. Replace > it by an ASSERT(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -1103,7 +1103,7 @@ static void set_eoi_ready(void *data); static void irq_guest_eoi_timer_fn(void *data) { struct irq_desc *desc = data; - unsigned int irq = desc - irq_desc; + unsigned int i, irq = desc - irq_desc; irq_guest_action_t *action; cpumask_t cpu_eoi_map; @@ -1114,19 +1114,18 @@ static void irq_guest_eoi_timer_fn(void action = (irq_guest_action_t *)desc->action; + ASSERT(action->ack_type != ACKTYPE_NONE); + if ( !action->in_flight || timer_is_active(&action->eoi_timer) ) goto out; - if ( action->ack_type != ACKTYPE_NONE ) + for ( i = 0; i < action->nr_guests; i++ ) { - unsigned int i; - for ( i = 0; i < action->nr_guests; i++ ) - { - struct domain *d = action->guest[i]; - unsigned int pirq = domain_irq_to_pirq(d, irq); - if ( test_and_clear_bool(pirq_info(d, pirq)->masked) ) - action->in_flight--; - } + struct domain *d = action->guest[i]; + unsigned int pirq = domain_irq_to_pirq(d, irq); + + if ( test_and_clear_bool(pirq_info(d, pirq)->masked) ) + action->in_flight--; } if ( action->in_flight )
action->ack_type is set once before the timer even gets initialized, and is never changed later. The timer gets activated only for EOI and UNMASK types. Hence there's no need to have a respective if() in there. Replace it by an ASSERT(). Signed-off-by: Jan Beulich <jbeulich@suse.com>