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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3fd88047-db3f-4165-9b58-fdeb5413c1a6@kernel.org>
Date: Tue, 30 Jul 2024 22:15:16 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: tj@...nel.org, cgroups@...r.kernel.org, shakeel.butt@...ux.dev,
 hannes@...xchg.org, lizefan.x@...edance.com, longman@...hat.com,
 kernel-team@...udflare.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V8 1/2] cgroup/rstat: Avoid flushing if there is an
 ongoing overlapping flush



On 30/07/2024 20.54, Yosry Ahmed wrote:
> [..]
>>>> +static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int cpu_in_loop,
>>>> +                                      bool already_contended)
>>>>           __acquires(&cgroup_rstat_lock)
>>>>    {
>>>> -       bool contended;
>>>> +       bool locked = false;
>>>>
>>>> -       contended = !spin_trylock_irq(&cgroup_rstat_lock);
>>>> -       if (contended) {
>>>> -               trace_cgroup_rstat_lock_contended(cgrp, cpu_in_loop, contended);
>>>> +       if (already_contended) /* Skip trylock if already contended */
>>>> +               locked = __cgroup_rstat_trylock(cgrp, cpu_in_loop);
>>> Should this be the other way around?
>>>
>> I think it is correct, but I used it wrong in once place, in
>> cgroup_rstat_flush_hold(), as cgroup_rstat_trylock_flusher() returning
>> false doesn't mean it was already_contended, but that ongoing flusher
>> "skipped" (and waited for) a flush.  I need to correct this.
>
> Something isn't adding up here as well. The comment says skip trylock
> if already contended, then if already_contended is true we do a
> trylock. Am I confusing myself here? 🙂

Your are correct. Thanks you for spelling it out for me!
I will send a V9 tomorrow, then deploy it to my prod experiment hosts,
and retest as I think my mistake here affects the prod results, as some
of the tracepoints gets skipped due to this.

Again thanks for catching this!!!
--Jesper

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ