[RFC PATCH 1/2] blk-mq: Convert request->csd to call_single_data_t and reposition it

[RFC PATCH 1/2] blk-mq: Convert request->csd to call_single_data_t and reposition it

[Leonardo Bras]

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.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 include/linux/blk-mq.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 06caacd77ed6..50ef86172621 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -189,16 +189,16 @@ struct request {
 		} flush;
 	};
 
-	union {
-		struct __call_single_data csd;
-		u64 fifo_time;
-	};
-
 	/*
 	 * completion callback.
 	 */
 	rq_end_io_fn *end_io;
 	void *end_io_data;
+
+	union {
+		call_single_data_t csd;
+		u64 fifo_time;
+	};
 };
 
 static inline enum req_op req_op(const struct request *req)
-- 
2.40.1

[RFC PATCH 2/2] smp: Change signatures to use call_single_data_t

[Leonardo Bras]

Every caller of smp_call_function_single_async() now makes use
of call_single_data_t, which is a size-aligned typedef of
struct __call_single_data.

Changing smp_call_function_single_async() csd parameter to
call_single_data_t makes possible to warn future callers if they
are using an unaligned csd, which can cause it to be split between 2
cachelines, which is usually bad for performance.

Also, for the same reason, change generic_exec_single().

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 kernel/smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index ab3e5dad6cfe..919387be6d4e 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -361,7 +361,7 @@ void __smp_call_single_queue(int cpu, struct llist_node *node)
  * for execution on the given CPU. data must already have
  * ->func, ->info, and ->flags set.
  */
-static int generic_exec_single(int cpu, struct __call_single_data *csd)
+static int generic_exec_single(int cpu, call_single_data_t *csd)
 {
 	if (cpu == smp_processor_id()) {
 		smp_call_func_t func = csd->func;
@@ -645,7 +645,7 @@ EXPORT_SYMBOL(smp_call_function_single);
  *
  * Return: %0 on success or negative errno value on error
  */
-int smp_call_function_single_async(int cpu, struct __call_single_data *csd)
+int smp_call_function_single_async(int cpu, call_single_data_t *csd)
 {
 	int err = 0;
 
-- 
2.40.1

Re: [RFC PATCH 1/2] blk-mq: Convert request->csd to call_single_data_t and reposition it

[Jens Axboe]

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. The
patch is also attempting to do two things at once, which is a bad idea.

Why not just rearrange it a bit so that we don't split a cacheline with
the csd?

-- 
Jens Axboe

Re: [RFC PATCH 1/2] blk-mq: Convert request->csd to call_single_data_t and reposition it

[Leonardo Brás]

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