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

[Anna-Maria Behnsen]

Sebastian Siewior <bigeasy@xxxxxxxxxxxxx> 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