On Fri, 2023-05-12 at 09:01 -0600, Jens Axboe wrote:
> On 5/11/23 2:58?AM, Leonardo Bras wrote:
> > Currently, request->csd has type struct __call_single_data.
> >
> > call_single_data_t is defined in include/linux/smp.h :
> >
> > /* Use __aligned() to avoid to use 2 cache lines for 1 csd */
> > typedef struct __call_single_data call_single_data_t
> > __aligned(sizeof(struct __call_single_data));
> >
> > As the comment above the typedef suggests, having this struct split between
> > 2 cachelines causes the need to fetch / invalidate / bounce 2 cachelines
> > instead of 1 when the cpu receiving the request gets to run the requested
> > function. This is usually bad for performance, due to one extra memory
> > access and 1 extra cacheline usage.
> >
> > Changing request->csd was previously attempted in commit
> > 966a967116e6 ("smp: Avoid using two cache lines for struct call_single_data")
> > but at the time the union that contains csd was positioned near the top of
> > struct request, only below a struct list_head, and this caused the issue of
> > holes summing up 24 extra bytes in struct request.
> >
> > The struct size was restored back to normal by
> > commit 4ccafe032005 ("block: unalign call_single_data in struct request")
> > but it caused the csd to be possibly split in 2 cachelines again.
> >
> > As an example with a 64-bit machine with
> > CONFIG_BLK_RQ_ALLOC_TIME=y
> > CONFIG_BLK_WBT=y
> > CONFIG_BLK_DEV_INTEGRITY=y
> > CONFIG_BLK_INLINE_ENCRYPTION=y
> >
> > Will output pahole with:
> > struct request {
> > [...]
> > union {
> > struct __call_single_data csd; /* 240 32 */
> > u64 fifo_time; /* 240 8 */
> > }; /* 240 32 */
> > [...]
> > }
> >
> > At this config, and any cacheline size between 32 and 256, will cause csd
> > to be split between 2 cachelines: csd->node (16 bytes) in the first
> > cacheline, and csd->func (8 bytes) & csd->info (8 bytes) in the second.
> >
> > During blk_mq_complete_send_ipi(), csd->func and csd->info are getting
> > changed, and when it calls __smp_call_single_queue() csd->node will get
> > changed.
> >
> > On the cpu which got the request, csd->func and csd->info get read by
> > __flush_smp_call_function_queue() and csd->node gets changed by
> > csd_unlock(), meaning the two cachelines containing csd will get accessed.
> >
> > To avoid this, it would be necessary to change request->csd back to
> > csd_single_data_t, which may end up increasing the struct size.
> > (In above example, it increased from 288 to 320 -> 32 bytes).
> >
> > In order to keep the csd_single_data_t and avoid the struct's size
> > increase, move request->csd to the end of the struct.
> > The rationale of this strategy is that for cachelines >= 32 bytes, there
> > will never be used an extra cacheline for struct request:
> >
> > - If request->csd is 32-byte aligned, there is no change in the object.
> > - If request->csd is not 32-byte aligned, and part of it is in a different
> > cacheline, the whole csd is moved to that cacheline.
> > - If request->csd is not 32-byte aligned, but it's all contained in the
> > same cacheline (> 32 bytes), aligning it to 32 will just put it a few
> > bytes forward in this cacheline.
> >
> > (In above example, the change kept the struct's size in 288 bytes).
> >
> > Convert request->csd to csd_single_data_t and move it to the end of
> > struct request, so csd is never split between cachelines and don't use any
> > extra cachelines.
>
> This is going the wrong way imho. It'd be nice to get struct request
> down to 256 bytes at some point, and then this would get in the way.
Hello Jens, thanks for reviewing!
I understand the idea, and I think we could have a way of rearranging the struct
in a way this variable would be always aligned, so no hole would be introduced
by the alignment requirement.
> The
> patch is also attempting to do two things at once, which is a bad idea.
By two things you mean rearranging the struct and switching the type to an
aligned typedef? I would be glad to send 2 patches on that.
>
> Why not just rearrange it a bit so that we don't split a cacheline with
> the csd?
>
Well, I thought that was exactly what I have done.
I mean, I was not aware of this future desire of shrinking it back to 256 bytes,
and so I added it to the end of the struct, but I could rearrange it in a
different way so the change to the 32-byte aligned call_single_data_t would
never introduce any size increase.
The thing with using call_single_data_t is that adding another struct field
before csd would cause struct request to increase by 32 bytes, and possibly grow
a hole, due to alignment.
This is good on the keeping the csd data in the same cacheline, but
this is bad because the struct would grow 32 bytes.
The other option, i.e. keeping 'struct __call_single_data', would cause the
struct to grow less if anything is added before, but would cause csd being
possibly shared between 2 cachelines.
I think the 1st option is 'better' because we could easily detect the 32-byte
increase in the struct request, but the 2-cachelines thing would probably go
undetected.
What you think about this reorder: put the csd union after 'struct block_device
*part', but before any compile-time removable field ?
This would cause the csd union to be 32-byte aligned with any CONFIG_* (as it
comes before any compile-time field in struct request), and in both 32-bit and
64-bit kernels
The pahole for this on 64-bit kernels outputs:
struct request {
[...]
struct block_device * part; /* 88 8 */
union {
call_single_data_t csd __attribute__((__aligned__(32)));
/* 96 32 */
[...]
/* size: 288, cachelines: 5, members: 33 */
/* sum members: 282, holes: 2, sum holes: 6 */
/* forced alignments: 2 */
/* last cacheline: 32 bytes */
} __attribute__((__aligned__(32)));
And for 32-bit kernels:
struct request {
[...]
struct block_device * part; /* 60 4 */
union {
call_single_data_t csd __attribute__((__aligned__(16)));
/* 64 16 */
u64 fifo_time; /* 64 8 */
} __attribute__((__aligned__(16))); /* 64 16 */
[...]
/* size: 192, cachelines: 3, members: 33 */
/* sum members: 178, holes: 1, sum holes: 2 */
/* padding: 12 */
/* forced alignments: 2 */
} __attribute__((__aligned__(16)));
What do you think?
Thanks!
Leo
