Re: [RFC PATCH 2/3] futex: Add basic infrastructure for local task local hash.

[Thomas Gleixner <tglx@linutronix.de>]

On Sun, Oct 27 2024 at 00:34, Sebastian Andrzej Siewior wrote:
>  static void futex_cleanup(struct task_struct *tsk)
>  {
> +	struct futex_hash_table *fht;
> +	bool need_free = false;
> +
>  	if (unlikely(tsk->robust_list)) {
>  		exit_robust_list(tsk);
>  		tsk->robust_list = NULL;
> @@ -1054,6 +1064,23 @@ static void futex_cleanup(struct task_struct *tsk)
>  
>  	if (unlikely(!list_empty(&tsk->pi_state_list)))
>  		exit_pi_state_list(tsk);
> +
> +	rcu_read_lock();
> +	fht = rcu_dereference(current->futex_hash_table);
> +	if (fht) {
> +
> +		spin_lock(&fht->lock);

I don't think you need a spin lock for this.

> +		fht->users--;
> +		WARN_ON_ONCE(fht->users < 0);
> +		if (fht->users == 0)
> +			need_free = true;
> +		spin_unlock(&fht->lock);
> +		rcu_assign_pointer(current->futex_hash_table, NULL);
> +	}
> +	rcu_read_unlock();

	scoped_guard (rcu) {
        	fht = rcu_dereference(current->futex_hash_table);
                if (fht && rcuref_put_rcusafe(&fht->refcnt)
			rcu_assign_pointer(current->futex_hash_table, NULL);
                else
                        fht = NULL;
	}
	kfree_rcu_mightsleep(fht);

Hmm? But see below

> +static int futex_hash_allocate(unsigned long arg3, unsigned long arg4,
> +			       unsigned long arg5)
> +{
> +	unsigned int hash_slots = arg3;
> +	struct futex_hash_table *fht;
> +	size_t struct_size;
> +	int i;
> +
> +	if (hash_slots == 0)
> +		hash_slots = 4;
> +	if (hash_slots < 2)
> +		hash_slots = 2;
> +	if (hash_slots > 16)
> +		hash_slots = 16;
> +	if (!is_power_of_2(hash_slots))
> +		hash_slots = rounddown_pow_of_two(hash_slots);
> +
> +	if (current->futex_hash_table)
> +		return -EALREADY;

You also need to check whether a hash table exists already in the
process. The table must be process shared to make sense. So it should
live in current->signal, which is a valid pointer in the context of
current.

So this needs some more thoughts especially vs. automatic creation and
sharing.

The first question is whether the hash table might have been already
created when the application reaches main(). If so then this call will
fail.

To make this work correctly, this needs proper serialization as well.

Assume a situation where the application does not allocate a table
upfront and the first futex use happens late when threads are running
already.

CPU0                            CPU1
T1                              T2        
sys_futex()                     sys_futex()
  create_hash()                   create_hash()

So T1 and T2 create their local hash and the subsequent usage will fail
because they operate on different hashs. You have the same problem
vs. your allocation scheme when two threads do prctl(ALLOC). We really
want to make this as simple as possible.

sys_futex()
   ...
   if (private)
      hash = private_hash(current);
   else
      hash = &global_hash;

private_hash()
   hash = READ_ONCE(current->signal->futex_hash);
   if (hash)
      return hash;
       
   guard(spinlock_irq)(&task->sighand->siglock);
   hash = current->signal->futex_hash;
   if (hash))
   	 return hash;

   hash = alloc_hash();
   WRITE_ONCE(current->signal->futex_hash, hash);
   return hash;

alloc_hash()
   if (!local_hash_enabled)
   	return &global_hash;
   hash = alloc();
   return hash ?: &global_hash;

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

        current->signal->futex_hash = NULL;
	if (hash == &global_hash)
           return;
   }

   kfree_rcu_mightsleep(hash);

Or something like that.

Thanks,

        tglx