linux-kernel - Re: [PATCH] percpu_counter: increase batch count

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]

Date:   Mon, 22 Feb 2021 14:53:55 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] percpu_counter: increase batch count

On 2/22/21 2:31 PM, Hugh Dickins wrote:
> On Thu, 18 Feb 2021, Jens Axboe wrote:
>> On 2/18/21 4:16 PM, Andrew Morton wrote:
>>> On Thu, 18 Feb 2021 14:36:31 -0700 Jens Axboe <axboe@...nel.dk> wrote:
>>>
>>>> Currently we cap the batch count at max(32, 2*nr_online_cpus), which these
>>>> days is kind of silly as systems have gotten much bigger than in 2009 when
>>>> this heuristic was introduced.
>>>>
>>>> Bump it to capping it at 256 instead. This has a noticeable improvement
>>>> for certain io_uring workloads, as io_uring tracks per-task inflight count
>>>> using percpu counters.
> 
> I want to quibble with the word "capping" here, it's misleading -
> but I'm sorry I cannot think of the right word.

Agree, it's not the best wording. And if you can't think of a better
one, then I'm at a loss too :-)

> The macro is max() not min(): you're making an improvement for
> certain io_uring workloads on machines with 1 to 15 cpus, right?
> Does "bigger than in 2009" apply to those?

Right, that actually had me confused. The box in question has 64 threads,
so my effective count was 128, or 256 with the patch.

> Though, io_uring could as well use percpu_counter_add_batch() instead?

That might be a simpler/better choice!

> (Yeah, this has nothing to do with me really, but I was looking at
> percpu_counter_compare() just now, for tmpfs reasons, so took more
> interest.  Not objecting to a change, but the wording leaves me
> wondering if the patch does what you think - or, not for the
> first time, I'm confused.)

I don't think you're confused, and honestly I think using the batch
version instead would likely improve our situation without potentially
changing behavior for everyone else. So it's likely the right way to go.

Thanks Hugh!

-- 
Jens Axboe