linux-kernel - Re: [PATCH 2/6] genirq: soft_moderation: add base files, procfs hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMOZA0+e8SqEeQTEa-6A7NPoCRMXOLF6xzt45+CRncJfhNDNEg@mail.gmail.com>
Date: Thu, 13 Nov 2025 23:32:48 +0100
From: Luigi Rizzo <lrizzo@...gle.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Marc Zyngier <maz@...nel.org>, Luigi Rizzo <rizzo.unipi@...il.com>, 
	Paolo Abeni <pabeni@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Sean Christopherson <seanjc@...gle.com>, Jacob Pan <jacob.jun.pan@...ux.intel.com>, 
	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH 2/6] genirq: soft_moderation: add base files, procfs hooks

On Thu, Nov 13, 2025 at 10:29 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> On Wed, Nov 12 2025 at 19:24, Luigi Rizzo wrote:
> >  [...]

most other comments will be addressed in the next version, but I wanted
to address a few of them here:

> > /sys/module/irq_moderation/parameters and read/write the procfs entries
> > /proc/irq/soft_moderation and /proc/irq/NN/soft_moderation.
> >
> > Examples:
> > cat /proc/irq/soft_moderation
> > echo "delay_us=345" > /proc/irq/soft_moderation
> > echo 1 | tee /proc/irq/*/nvme*/../soft_moderation
>
> That's impressive to be able to write into proc/...

That is already the case for /proc/irq/default_smp_affinity and
/proc/irq/NN/smp_affinity[_list] so I followed the same approach.

> > +
> > +/* After the handler, if desc is moderated, make sure the timer is active. */
> > +static inline void irq_moderation_epilogue(const struct irq_desc *desc)
> > +{
> > +     struct irq_mod_state *ms = this_cpu_ptr(&irq_mod_state);
>
> Lacks a check whether moderation is enabled or not.

That would be redundant, !list_empty(desc->ms_node) is only true
if moderation is enabled and many other conditions in the previous hook.

> > +             /* Timer still alive. Just call the handlers */
> > +             list_for_each_entry_safe(desc, next, &ms->descs, ms_node) {
> > +                     ms->irq_count += irq_mod_info.count_timer_calls;
> > +                     ms->timer_calls++;
> > +                     handle_irq_event_percpu(desc);
>
> That lacks setting the INPROGRESS flag.

I did it in an earlier test version, but was wondering if that is
actually needed.
While the irqdesc is in the timer list, irqd_irq_disabled() is true and that
makes irq_can_handle_actions() and irq_can_handle() return false.

> > +#pragma clang diagnostic error "-Wformat"
>
> What's that for?

print() formats with a lot of entries are error prone, and the above
catches a lot of trivial mistakes, so I felt it was useful to keep it.

cheers
luigi