On Wed, Nov 15 2023 at 13:56, Peter Zijlstra wrote:
>
> Would it not make more sense to write things something like:
>
> bool handle_pending_pir()
> {
> bool handled = false;
> u64 pir_copy[4];
>
> for (i = 0; i < 4; i++) {
> if (!pid-pir_l[i]) {
> pir_copy[i] = 0;
> continue;
> }
>
> pir_copy[i] = arch_xchg(&pir->pir_l[i], 0);
> handled |= true;
> }
>
> if (!handled)
> return handled;
>
> for_each_set_bit()
> ....
>
> return handled.
> }
I don't understand what the whole copy business is about. It's
absolutely not required.
static bool handle_pending_pir(unsigned long *pir)
{
unsigned int idx, vec;
bool handled = false;
unsigned long pend;
for (idx = 0; offs < 4; idx++) {
if (!pir[idx])
continue;
pend = arch_xchg(pir + idx, 0);
for_each_set_bit(vec, &pend, 64)
call_irq_handler(vec + idx * 64, NULL);
handled = true;
}
return handled;
}
No?
> sysvec_posted_blah_blah()
> {
> bool done = false;
> bool handled;
>
> for (;;) {
> handled = handle_pending_pir();
> if (done)
> break;
> if (!handled || ++loops > MAX_LOOPS) {
That does one loop too many. Should be ++loops == MAX_LOOPS. No?
> pi_clear_on(pid);
> /* once more after clear_on */
> done = true;
> }
> }
> }
>
>
> Hmm?
I think that can be done less convoluted.
{
struct pi_desc *pid = this_cpu_ptr(&posted_interrupt_desc);
struct pt_regs *old_regs = set_irq_regs(regs);
int loops;
for (loops = 0;;) {
bool handled = handle_pending_pir((unsigned long)pid->pir);
if (++loops > MAX_LOOPS)
break;
if (!handled || loops == MAX_LOOPS) {
pi_clear_on(pid);
/* Break the loop after handle_pending_pir()! */
loops = MAX_LOOPS;
}
}
...
set_irq_regs(old_regs);
}
Hmm? :)
