public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Sebastian Andrzej Siewior" <bigeasy@linutronix.de>,
	linux-kernel@vger.kernel.org,
	"André Almeida" <andrealmeid@igalia.com>,
	"Darren Hart" <dvhart@infradead.org>,
	"Davidlohr Bueso" <dave@stgolabs.net>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Juri Lelli" <juri.lelli@redhat.com>,
	"Valentin Schneider" <vschneid@redhat.com>,
	"Waiman Long" <longman@redhat.com>
Subject: Re: [RFC PATCH 2/3] futex: Add basic infrastructure for local task local hash.
Date: Mon, 28 Oct 2024 13:02:34 +0100	[thread overview]
Message-ID: <87r080306d.ffs@tglx> (raw)
In-Reply-To: <20241028110035.GQ9767@noisy.programming.kicks-ass.net>

On Mon, Oct 28 2024 at 12:00, Peter Zijlstra wrote:
> On Mon, Oct 28, 2024 at 11:58:18AM +0100, Thomas Gleixner wrote:
>> > Let me post v2 the signal_struct and then think about auto ON.
>> 
>> It only affects actual futex users. A lot of executables never use
>> them. For ease of testing, can you please make this automatic so there
>> is no need to modify a test case?
>> 
>> Aside of that for RT we really want it automatically enabled and as
>> Linus suggested back then probably for NUMA too.
>> 
>> Stick a trace point or a debugfs counter into the allocation so you can
>> observe how many of those are actually allocated and used concurrently.
>
> Ideally it would re-hash and auto-scale to something like 4*nr_threads,
> but I realize that's probably a pain in the arse to make happen.

That's what we did with the original series, but with this model it's
daft. What we maybe could do there is:

private_hash()
   scoped_guard(rcu) {
      hash = rcu_dereference(current->signal->futex_hash);
      if (hash && rcuref_get(&hash->ref))
         return hash;
   }

   guard(spinlock_irq)(&task->sighand->siglock);
   hash = current->signal->futex_hash;
   if (hash && rcuref_get(&hash->ref))
       return hash;
   // Let alloc scale according to signal->nr_threads
   // alloc acquires a reference count
   ....

And on fork do the following:

   scoped_guard(spinlock_irq, &task->sighand->siglock) {
      hash = current->signal->futex_hash;
      if (!hash || hash_size_ok())
   	return hash;

      // Drop the initial reference, which forces the last
      // user and subsequent new users into the respective
      // slow paths, where they get stuck on sighand lock.
      if (!rcuref_put(&hash->ref))
        return;

      // rcuref_put() dropped the last reference
      old_hash = realloc_hash(hash);
      hash = current->signal->futex_hash;
   }
   kfree_rcu(old_hash);
   return hash;

A similar logic is required when putting the last reference

futex_hash_put()
{
   if (!rcuref_put(&hash->ref))
      return;

   scoped_guard(spinlock_irq, &task->sighand->siglock) {
      // Fork might have raced with this
      if (hash != current->signal->futex_hash)
      	 return;
      old_hash = realloc_hash(hash);
   }
   kfree_rcu(old_hash);  
}

realloc_hash(old_hash)
{
   new_hash = alloc():
   if (!new_hash) {
      // Make the old hash alive again
      rcuref_init(&old_hash->ref);
      return NULL;
   }
   rehash(old_hash, new_hash);
   rcu_assign_pointer(current->signal->new_hash);
   return old_hash;
}

Or something like that. On the usage site this needs

   // Takes a reference count on the hash
   hb = futex_hash(key);

   lock(hb);
   queue();
   unlock(hb);
   futex_hash_put(hb);

which means, after the put @hb is not longer valid as the rehashing
might happen right in the put or afterwards. That needs some auditing of
the usage sites, but that should work. Whether it's worth it is a
different question.

Thanks,

        tglx

  reply	other threads:[~2024-10-28 12:02 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-26 22:34 [RFC PATCH 0/3] futex: Add support task local hash maps Sebastian Andrzej Siewior
2024-10-26 22:34 ` [RFC PATCH 1/3] futex: Create helper function to initialize a hash slot Sebastian Andrzej Siewior
2024-10-26 22:34 ` [RFC PATCH 2/3] futex: Add basic infrastructure for local task local hash Sebastian Andrzej Siewior
2024-10-27 12:19   ` Thomas Gleixner
2024-10-28 10:30     ` Sebastian Andrzej Siewior
2024-10-28 10:58       ` Thomas Gleixner
2024-10-28 11:00         ` Peter Zijlstra
2024-10-28 12:02           ` Thomas Gleixner [this message]
2024-10-30 21:08             ` Peter Zijlstra
2024-10-30 23:14               ` Thomas Gleixner
2024-10-31  9:13                 ` Peter Zijlstra
2024-10-28 10:16   ` Peter Zijlstra
2024-10-28 10:24     ` Sebastian Andrzej Siewior
2024-10-28 10:46       ` Peter Zijlstra
2024-10-28 13:10         ` Peter Zijlstra
2024-10-26 22:34 ` [RFC PATCH 3/3] futex: Use the task local hashmap Sebastian Andrzej Siewior
2024-10-28 10:22   ` Peter Zijlstra
2024-10-28 10:24     ` Sebastian Andrzej Siewior
2024-10-27 10:01 ` [RFC PATCH 0/3] futex: Add support task local hash maps Thomas Gleixner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r080306d.ffs@tglx \
    --to=tglx@linutronix.de \
    --cc=andrealmeid@igalia.com \
    --cc=bigeasy@linutronix.de \
    --cc=dave@stgolabs.net \
    --cc=dvhart@infradead.org \
    --cc=juri.lelli@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=vschneid@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox