linux-kernel - Re: [PATCH v9 30/32] timers: Implement the hierarchical pull model

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: <87y1dzlbh8.fsf@somnus>
Date:   Tue, 12 Dec 2023 15:52:19 +0100
From:   Anna-Maria Behnsen <anna-maria@...utronix.de>
To:     Sebastian Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        John Stultz <jstultz@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Eric Dumazet <edumazet@...gle.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Arjan van de Ven <arjan@...radead.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Rik van Riel <riel@...riel.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Giovanni Gherdovich <ggherdovich@...e.cz>,
        Lukasz Luba <lukasz.luba@....com>,
        "Gautham R . Shenoy" <gautham.shenoy@....com>,
        Srinivas Pandruvada <srinivas.pandruvada@...el.com>,
        K Prateek Nayak <kprateek.nayak@....com>
Subject: Re: [PATCH v9 30/32] timers: Implement the hierarchical pull model

Sebastian Siewior <bigeasy@...utronix.de> writes:

> On 2023-12-01 10:26:52 [+0100], Anna-Maria Behnsen wrote:

[...]

>> diff --git a/kernel/time/timer_migration.h b/kernel/time/timer_migration.h
>> new file mode 100644
>> index 000000000000..260b87e5708d
>> --- /dev/null
>> +++ b/kernel/time/timer_migration.h
>> @@ -0,0 +1,144 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef _KERNEL_TIME_MIGRATION_H
>> +#define _KERNEL_TIME_MIGRATION_H
>> +
>> +/* Per group capacity. Must be a power of 2! */
>> +#define TMIGR_CHILDREN_PER_GROUP 8
>
> BUILD_BUG_ON_NOT_POWER_OF_2(TMIGR_CHILDREN_PER_GROUP)
>
> Maybe in the .c file.
>

in tmigr_init() ?

>> +/**
>> + * struct tmigr_event - a timer event associated to a CPU
>> + * @nextevt:	The node to enqueue an event in the parent group queue
>> + * @cpu:	The CPU to which this event belongs
>> + * @ignore:	Hint whether the event could be ignored; it is set when
>> + *		CPU or group is active;
>> + */
>> +struct tmigr_event {
>> +	struct timerqueue_node	nextevt;
>> +	unsigned int		cpu;
>> +	bool			ignore;
>> +};
>> +
>> +/**
>> + * struct tmigr_group - timer migration hierarchy group
>> + * @lock:		Lock protecting the event information and group hierarchy
>> + *			information during setup
>> + * @migr_state:		State of the group (see union tmigr_state)
>
> So the lock does not protect migr_state?

Right - this is not required due to the atomic cmpxchg and seqence
counter.

> Mind moving it a little down the road? Not only would it be more
> obvious what is protected by the lock but it would also move
> migr_state in another/ later cache line.
>

Where do you want me to move it? Switch places of lock and migr_state?
When I put it to another place, I would generate holes. A general
question: Is it required to have a look at the struct with pahole also
with LOCKDEP enabled? If yes, lock should stay at the first position.

[...]

>> + * @cpuevt:		CPU event which could be queued into the parent group
>
> I don't know why but it feels like s/queued/enqueued/g
> But it might be a British vs American thing.

I think this queued/enqueued is not used consistent all over the place
(in my patchset). But I'm also not a native speaker and not sure which
is the proper one :). Nevertheless, I'll change it.

Thanks,

        Anna-Maria