Re: [PATCH 2/2] getrusage: use sig->stats_lock

[Oleg Nesterov <oleg@redhat.com>]

On 01/20, Andrew Morton wrote:
>
> On Fri, 19 Jan 2024 19:27:49 -0800 Dylan Hatch <dylanbhatch@google.com> wrote:
>
> >
> > I applied these to a 5.10 kernel, and my repro (calling getrusage(RUSAGE_SELF)
> > from 200K threads) is no longer triggering a hard lockup.
>
> Thanks, but...
>
> The changelogs don't actually describe any hard lockup.  [1/2] does
> mention "the deadlock" but that's all the info we have.

Sorry for confusion... 1/2 tries to explain that this change is not
strictly necessary for 2/2, it is safe to call thread_group_cputime()
with sig->stats_lock held for writing even if thread_group_cputime()
takes the same lock, because in this case thread_group_cputime() can't
enter the slow mode.

> So could we please have a suitable description of the bug which these are
> addressing?  And a Reported-by:, a Closes: and a Fixes would be great too.

Yes sorry I forgot to add Reported-by. So I'll try to update the changelog
and add Reported-and-tested-by.

But the problem is known and old. I think do_io_accounting() had the same
problem until 1df4bd83cdfdbd0 ("do_io_accounting: use sig->stats_lock").
and do_task_stat() ...

getrusage() takes siglock and does for_each_thread() twice. If NR_THREADS
call sys_getrusage() in an endless loop on NR_CPUS, lock_task_sighand()
can trigger a hard lockup because it spins with irqs disabled waiting
for other NR_CPUS-1 which need the same siglock. So the time it spins
with irqs disabled is O(NR_CPUS * NR_THREADS).

With this patch all the threads can run lockless in parallel in the
likely case.

Dylan, do you have a better description? Can you share your repro?
although I think that something simple like

	#define NT BIG_NUMBER

	pthread_barrier_t barr;

	void *thread(void *arg)
	{
		struct rusage ru;

		pthread_barrier_wait(&barr);
		for (;;)
			getrusage(RUSAGE_SELF, &ru);
		return NULL;
	}

	int main(void)
	{
		pthread_barrier_init(&barr, NULL, NT);

		for (int n = 0; n < NT-1; ++n) {
			pthread_t pt;
			pthread_create(&pt, NULL, thread, NULL);
		}
		thread(NULL);

		return 0;
	}

should work if you have a machine with a lot of memory/cpus.

Oleg.