linux-kernel - Re: [PATCH v4 3/3] genirq: Adaptive Global Software Interrupt Moderation (GSIM).

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: <87pl7a345y.ffs@tglx>
Date: Thu, 15 Jan 2026 23:24:41 +0100
From: Thomas Gleixner <tglx@...nel.org>
To: Luigi Rizzo <lrizzo@...gle.com>, 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>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org, Bjorn Helgaas
 <bhelgaas@...gle.com>, Willem de Bruijn <willemb@...gle.com>, Luigi Rizzo
 <lrizzo@...gle.com>
Subject: Re: [PATCH v4 3/3] genirq: Adaptive Global Software Interrupt
 Moderation (GSIM).

On Thu, Jan 15 2026 at 15:59, Luigi Rizzo wrote:
>  /* Initialize moderation state, used in desc_set_defaults() */
>  void irq_moderation_init_fields(struct irq_desc_mod *mod_state)
>  {
> @@ -189,7 +379,9 @@ static int swmod_wr_u32(struct swmod_param *n, const char __user *s, size_t coun
>  	int ret = kstrtouint_from_user(s, count, 0, &res);
>  
>  	if (!ret) {
> +		write_seqlock(&irq_mod_info.seq);

That's broken.

              spin_lock(seq->lock);
              seq->seqcount++;
Interrupt
              ...
              do {
              	read_seqbegin(seq)
                    --> livelock because seqcount is odd

You clearly never ran that code with lockdep enabled because lockdep
would have yelled at you. Testing patches with lockdep is not optional
as documented...

And no, using write_seqlock_irq() won't fix it either as that will still
explode on a RT enabled kernel as documented...

Also this sequence count magic on the read side does not have any real
value:

> +	if (raw_read_seqcount(&irq_mod_info.seq.seqcount) != m->seq) {

Q: What's the gain of this sequence count magic?

A: Absolutely nothing. Once this pulled the cache line in, then the
   sequence count magic is just cargo cult programming because:

      1) The data is a snapshot in any case as the next update might
         come right after that

      2) Once the cache line is pulled in, reading the _three_
         parameters from it (assumed they have been already converted to
         nanoseconds) is basically free and definitely less expensive
         than the seqbegin/retry loop which comes with expensive SMP
         barriers on some architectures.

Sequence counts are only useful when you need a consistent data set and
the write side updates the whole data set in one go.

As this is a write one by one operation, there is no consistency problem
and the concurrency is handled nicely by READ/WRITE_ONCE(). Either it
sees the old value or the new.

If you actually update the per CPU instances in the parameter write
functions, you can avoid touching the global cache line completely, no?

Can you please stop polishing your prove of concept code to death and
actually sit back and think about a proper design and implementation?

I don't care at all about you wasting _your_ time, but I very much care
about _my_ time wasted by this.

Thanks,

        tglx