[bpf-next,4/6] xsk: carry a copy of xdp_zc_max_segs within xsk_buff_pool

Commit Message

Maciej Fijalkowski Oct. 2, 2024, 3:54 p.m. UTC

This so we avoid dereferencing struct net_device within hot path.

Signed-off-by: Maciej Fijalkowski <[e-mail]>
---
 include/net/xsk_buff_pool.h | 1 +
 net/xdp/xsk_buff_pool.c     | 1 +
 net/xdp/xsk_queue.h         | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

Comments

Vadim Fedorenko Oct. 2, 2024, 7:41 p.m. UTC | #1

On 02/10/2024 16:54, Maciej Fijalkowski wrote:
> This so we avoid dereferencing struct net_device within hot path.
> 
> Signed-off-by: Maciej Fijalkowski <[e-mail]>
> ---
>   include/net/xsk_buff_pool.h | 1 +
>   net/xdp/xsk_buff_pool.c     | 1 +
>   net/xdp/xsk_queue.h         | 2 +-
>   3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> index 468a23b1b4c5..8223581d95f8 100644
> --- a/include/net/xsk_buff_pool.h
> +++ b/include/net/xsk_buff_pool.h
> @@ -77,6 +77,7 @@ struct xsk_buff_pool {
>   	u32 chunk_shift;
>   	u32 frame_len;
>   	u8 tx_metadata_len; /* inherited from umem */
> +	u32 xdp_zc_max_segs;

It's better not to make holes in the struct. And looks like it's better
to move it closer to free_list_cnt to put it on the same cache line with
tx_descs which is accessed earlier in xskq_cons_read_desc_batch()
(though the last point is not strict because both cache lines should be
hot at the moment)

>   	u8 cached_need_wakeup;
>   	bool uses_need_wakeup;
>   	bool unaligned;

Maciej Fijalkowski Oct. 3, 2024, 11:47 a.m. UTC | #2

On Wed, Oct 02, 2024 at 08:41:33PM +0100, Vadim Fedorenko wrote:
> On 02/10/2024 16:54, Maciej Fijalkowski wrote:
> > This so we avoid dereferencing struct net_device within hot path.
> > 
> > Signed-off-by: Maciej Fijalkowski <[e-mail]>
> > ---
> >   include/net/xsk_buff_pool.h | 1 +
> >   net/xdp/xsk_buff_pool.c     | 1 +
> >   net/xdp/xsk_queue.h         | 2 +-
> >   3 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> > index 468a23b1b4c5..8223581d95f8 100644
> > --- a/include/net/xsk_buff_pool.h
> > +++ b/include/net/xsk_buff_pool.h
> > @@ -77,6 +77,7 @@ struct xsk_buff_pool {
> >   	u32 chunk_shift;
> >   	u32 frame_len;
> >   	u8 tx_metadata_len; /* inherited from umem */
> > +	u32 xdp_zc_max_segs;
> 
> It's better not to make holes in the struct. And looks like it's better
> to move it closer to free_list_cnt to put it on the same cache line with
> tx_descs which is accessed earlier in xskq_cons_read_desc_batch()
> (though the last point is not strict because both cache lines should be
> hot at the moment)

Hi Vadim,

Yeah I agree placement is awkward, i'll move this above tx_metadata_len to
keep booleans together. CL is correct though as ::unaligned is accessed
when parsing/validating descs so as you said, both these CLs are supposed
to be hot.

Thanks for review! Will send a v2.

> 
> >   	u8 cached_need_wakeup;
> >   	bool uses_need_wakeup;
> >   	bool unaligned;
> 
>

Patch

diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
index 468a23b1b4c5..8223581d95f8 100644
--- a/include/net/xsk_buff_pool.h
+++ b/include/net/xsk_buff_pool.h
@@ -77,6 +77,7 @@  struct xsk_buff_pool {
 	u32 chunk_shift;
 	u32 frame_len;
 	u8 tx_metadata_len; /* inherited from umem */
+	u32 xdp_zc_max_segs;
 	u8 cached_need_wakeup;
 	bool uses_need_wakeup;
 	bool unaligned;
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 7ecd4ccd2473..e946ba4a5ccf 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -229,6 +229,7 @@  int xp_assign_dev(struct xsk_buff_pool *pool,
 		goto err_unreg_xsk;
 	}
 	pool->umem->zc = true;
+	pool->xdp_zc_max_segs = netdev->xdp_zc_max_segs;
 	return 0;
 
 err_unreg_xsk:
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 406b20dfee8d..46d87e961ad6 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -260,7 +260,7 @@  u32 xskq_cons_read_desc_batch(struct xsk_queue *q, struct xsk_buff_pool *pool,
 			nr_frags = 0;
 		} else {
 			nr_frags++;
-			if (nr_frags == pool->netdev->xdp_zc_max_segs) {
+			if (nr_frags == pool->xdp_zc_max_segs) {
 				nr_frags = 0;
 				break;
 			}