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

[Thomas Gleixner <tglx@linutronix.de>]

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.

> +/* Per-CPU state initialization */
> +static void irq_moderation_percpu_init(void *data)
> +{
> +	struct irq_mod_state *ms = this_cpu_ptr(&irq_mod_state);
> +
> +	INIT_LIST_HEAD(&ms->descs);
> +}
> +
> +static int cpuhp_setup_cb(uint cpu)
> +{
> +	irq_moderation_percpu_init(NULL);
> +	return 0;
> +}
> +
> +static void clamp_parameter(uint *dst, uint val)
> +{
> +	struct param_names *n = param_names;
> +	uint i;
> +
> +	for (i = 0; i < ARRAY_SIZE(param_names); i++, n++) {
> +		if (dst == n->val) {
> +			*dst = clamp(val, n->min, n->max);
> +			return;
> +		}
> +	}
> +}
> +
> +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.

> +	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "moderation:online", cpuhp_setup_cb, NULL);

> +#ifdef CONFIG_IRQ_SOFT_MODERATION
> +
> +/**
> + * struct irq_mod_info - global configuration parameters and state
> + * @total_intrs:	running count updated every update_ms
> + * @total_cpus:		as above, active CPUs in this interval
> + * @procfs_write_ns:	last write to /proc/irq/soft_moderation
> + * @delay_us:		fixed delay, or maximum for adaptive
> + * @target_irq_rate:	target maximum interrupt rate
> + * @hardirq_percent:	target maximum hardirq percentage
> + * @timer_rounds:	how many timer polls once moderation fires
> + * @update_ms:		how often to update delay/rate/fraction
> + * @scale_cpus:		(percent) scale factor to estimate active CPUs
> + * @count_timer_calls:	count timer calls for irq limits
> + * @count_msi_calls:	count calls from posted_msi for irq limits
> + * @decay_factor:	smoothing factor for the control loop, keep at 16
> + * @grow_factor:	smoothing factor for the control loop, keep it at 8
> + */
> +struct irq_mod_info {
> +	/* These fields are written to by all CPUs */
> +	____cacheline_aligned
> +	atomic_long_t total_intrs;
> +	atomic_long_t total_cpus;
> +
> +	/* These are mostly read (frequently), so use a different cacheline */
> +	____cacheline_aligned
> +	u64 procfs_write_ns;
> +	uint delay_us;

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.

Feel free to ignore my feedback, but don't be upset if I ignore your
patches then.

Thanks,

        tglx