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

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: <CAMOZA0JwGT946kAZYq_s3W6LQxwKAAqOGgtCWOYzomEZu4uYcg@mail.gmail.com>
Date: Mon, 17 Nov 2025 17:16:32 +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 v2 2/8] genirq: soft_moderation: add base files, procfs

On Mon, Nov 17, 2025 at 5:01 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> On Sun, Nov 16 2025 at 18:28, Luigi Rizzo wrote:
> > +/* Handlers for /proc/irq/NN/soft_moderation */
> > +static int mode_show(struct seq_file *p, void *v)
> > +{
> > +     struct irq_desc *desc = p->private;
> > +
> > +     if (!desc)
> > +             return -ENOENT;
> > +
> > +     seq_printf(p, "%s irq %u trigger 0x%x %s %smanaged %slazy handle_irq %pB\n",
> > +                desc->mod.enable ? "on" : "off", desc->irq_data.irq,
> > +                irqd_get_trigger_type(&desc->irq_data),
> > +                irqd_is_level_type(&desc->irq_data) ? "Level" : "Edge",
> > +                irqd_affinity_is_managed(&desc->irq_data) ? "" : "un",
> > +                irq_settings_disable_unlazy(desc) ? "un" : "", desc->handle_irq
>
> Why are you exposing randomly picked information here? The only thing
> what's interesting is whether the interrupt is moderated or not. The
> interrupt number is redundant information. And all the other internal
> details are available in debugfs already.

ah, sorry, forgot to clean up this internal debugging code.
The previous version had only the enable.

>
> >
> > +static int __init init_irq_moderation(void)
> > +{
> > +     uint *cur;
> > +
> > +     on_each_cpu(irq_moderation_percpu_init, NULL, 1);
>
> That's pointless. Register the hotplug callback without 'nocalls' and
> let the hotplug code handle it.

Sounds good. I have a question on event ordering.
Which event should I use to make sure the callback runs
before interrupts are enabled on the new CPU ?

> > ...
> I asked you last time already to follow the TIP tree documentation, no?
>
> > +     uint target_irq_rate;
> > +     uint hardirq_percent;
> > +     uint timer_rounds;
> > +     uint update_ms;
> > +     uint scale_cpus;
> > +     uint count_timer_calls;
> > +     uint count_msi_calls;
> > +     uint decay_factor;
> > +     uint grow_factor;
> > +     uint pad[];
> > +};
>
> And again you decided to add these fat data structures all in once with
> no usage. I told you last time that this is unreviewable and that stuff
> should be introduced gradually with the usage.

Ok, will do.
FWIW my goal was to get the telemetry functions in the first patch, and reduce
the clutter in subsequent patches, since each new field would create many
chunks (docstring, struct field, init, print format and value).

cheers
luigi