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

[Sebastian Siewior <bigeasy@linutronix.de>]

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