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

Sebastian Siewior
2023-12-12T17:09:03Z

On 2023-12-12 15:52:19 [+0100], Anna-Maria Behnsen wrote:
> Sebastian Siewior <bigeasy@linutronix.de> writes:
>
> >> +/* 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() ?

Yeah why not. It is used there for the init of the structs.

> >> +/**
> >> + * 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.

Maybe something like:
| struct tmigr_group {
| raw_spinlock_t lock; /* 0 4 */
|
| /* XXX 4 bytes hole, try to pack */
|
| struct tmigr_group * parent; /* 8 8 */
| struct tmigr_event groupevt __attribute__((__aligned__(8))); /* 16 40 */
|
| /* XXX last struct has 3 bytes of padding */
|
| u64 next_expiry; /* 56 8 */
| /* --- cacheline 1 boundary (64 bytes) --- */
| struct timerqueue_head events; /* 64 16 */
| atomic_t migr_state; /* 80 4 */
| unsigned int level; /* 84 4 */
| int numa_node; /* 88 4 */
| unsigned int num_children; /* 92 4 */
| u8 childmask; /* 96 1 */
|
| /* XXX 7 bytes hole, try to pack */
|
| struct list_head list; /* 104 16 */

Starting with lock isn't bad as you see everything from here is
protected by lock. If it makes sense you could start with list so that
the container_of() becomes a NOP.
I wouldn't make lockdep a thing and assume it is off. Also, I would
assume the architecture is 64bit.

However with lockdep enabled it becomes:

| struct tmigr_group {
| raw_spinlock_t lock; /* 0 64 */
| /* --- cacheline 1 boundary (64 bytes) --- */
| struct tmigr_group * parent; /* 64 8 */
| struct tmigr_event groupevt __attribute__((__aligned__(8))); /* 72 40 */
|
| /* XXX last struct has 3 bytes of padding */
|
| u64 next_expiry; /* 112 8 */
| struct timerqueue_head events; /* 120 16 */
| /* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */
| atomic_t migr_state; /* 136 4 */
| unsigned int level; /* 140 4 */
| int numa_node; /* 144 4 */
| unsigned int num_children; /* 148 4 */
| u8 childmask; /* 152 1 */
|
| /* XXX 7 bytes hole, try to pack */
|
| struct list_head list; /* 160 16 */
| } __attribute__((__aligned__(8)));

so it didn't change much.

I shuffled it a bit and everything after migr_state is read only.
I don't think looking at pahole is required but in your case it makes
sense to put the locked section into a separate cache line vs migr_state
variable. It doesn't cost much.
You can decide if it is worth to move childmask after the lock so you so
you avoid the 7 byte hole at the end. I wouldn't do it to satisfy pahole
here. If it makes sense, doesn't hurt/ confuse why not.

You would consider the pahole output more on a structure like dentry
which is used a _lot_. So saving 4 bytes would mean save a megabyte or
ten in the end.

> Thanks,
>
> Anna-Maria
>

Sebastian