netdev - Re: [PATCH bpf-next 4/6] xsk: carry a copy of xdp_zc_max_segs within xsk_buff

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Zv6EZybVMBeHluAl@boxer>
Date: Thu, 3 Oct 2024 13:47:51 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
CC: <bpf@...r.kernel.org>, <ast@...nel.org>, <daniel@...earbox.net>,
	<andrii@...nel.org>, <netdev@...r.kernel.org>, <magnus.karlsson@...el.com>,
	<bjorn@...nel.org>
Subject: Re: [PATCH bpf-next 4/6] xsk: carry a copy of xdp_zc_max_segs within
 xsk_buff_pool

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 <maciej.fijalkowski@...el.com>
> > ---
> >   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;
> 
>