Re: [RFC PATCH v2 2/4] irq/spurious: Account for multiple handles in note_interrupt

[Leonardo Bras <leobras@redhat.com>]

On Fri, Feb 16, 2024 at 05:36:37PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 16, 2024 at 04:59:44AM -0300, Leonardo Bras wrote:
> > Currently note_interrupt() will check threads_handled for changes and use
> > it to mark an IRQ as handled, in order to avoid having a threaded
> > interrupt to falsely trigger unhandled IRQ detection.
> > 
> > This detection can still be falsely triggered if we have many IRQ handled
> > accounted between each check of threads_handled, as those will be counted
> > as a single one in the unhandled IRQ detection.
> > 
> > In order to fix this, subtract from irqs_unhandled the number of IRQs
> > handled since the last check (threads_handled_last - threads_handled).
> 
> ...
> 
> > +static inline int get_handled_diff(struct irq_desc *desc)
> > +{
> > +	unsigned int handled;
> > +	int diff;
> > +
> > +	handled = (unsigned int)atomic_read(&desc->threads_handled);
> > +	handled |= SPURIOUS_DEFERRED;
> > +
> > +	diff = handled - desc->threads_handled_last;
> > +	diff >>= SPURIOUS_DEFERRED_SHIFT;
> > +
> > +	/*
> > +	 * Note: We keep the SPURIOUS_DEFERRED bit set. We are handling the
> > +	 * previous invocation right now. Keep it for the current one, so the
> > +	 * next hardware interrupt will account for it.
> > +	 */
> 

Hello Andy, thanks for reviewing!

> > +	if (diff != 0)
> 
> 	if (diff)

Sure

> 
> > +		desc->threads_handled_last = handled;
> > +
> > +	return diff;
> > +}
> 
> ...
> 
> > +			diff = get_handled_diff(desc);
> > +			if (diff > 0) {
> 
> diff may not be negative as you always right shift by 1 (or more) bit.

Agree

> Hence
> 
> 			if (diff)
> 
> will suffice (also be aligned with the similar check inside the helper) and
> making the helper to return unsigned value will be clearer. Am I correct?

Sure, you are correct.

I just think having it be (diff > 0) makes it clear that we only do the 
subtraction if diff is bigger than zero, while (diff) could mean diff being 
valid, and would require the reader to go back in code to see that diff is 
an int. 

Does it make sense?

Other than that, I agree the negative half of diff is never going to get 
used, and it's better to go with unsigned int in both cases.

That will be changed on the next version.

Thanks!
Leo

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>