On Thu, Nov 24, 2022 at 03:39:20PM +0100, Toke Høiland-Jørgensen wrote:

Jakub Kicinski [off-list ref] writes:

quoted

On Wed, 23 Nov 2022 22:55:21 +0100 Toke Høiland-Jørgensen wrote:

quoted

quoted

Good idea, prototyped below, lmk if it that's not what you had in mind.

struct xdp_buff_xsk {
	struct xdp_buff            xdp;                  /*     0    56 */
	u8                         cb[16];               /*    56    16 */
	/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */  

As pahole helpfully says here, xdp_buff is actually only 8 bytes from
being a full cache line. I thought about adding a 'cb' field like this
to xdp_buff itself, but figured that since there's only room for a
single pointer, why not just add that and let the driver point it to
where it wants to store the extra context data?

What if the driver wants to store multiple pointers or an integer or
whatever else? The single pointer seems quite arbitrary and not
strictly necessary.

Well, then you allocate a separate struct and point to that? Like I did
in mlx5:


+	struct mlx5_xdp_ctx mlctx = { .cqe = cqe, .rq = rq };
+	struct xdp_buff xdp = { .drv_priv = &mlctx };

but yeah, this does give an extra pointer deref on access. I'm not
really opposed to the cb field either, I just think it's a bit odd to
put it in struct xdp_buff_xsk; that basically requires the driver to
keep the layouts in sync.

Instead, why not but a cb field into xdp_buff itself so it can be used
for both the XSK and the non-XSK paths? Then the driver can just
typecast the xdp_buff into its own struct that has whatever data it
wants in place of the cb field?

Why can't you simply have a pointer to xdp_buff in driver specific
xdp_buff container which would point to xdp_buff that is stack based (or
whatever else memory that will back it up - I am about to push a change
that makes ice driver embed xdp_buff within a struct that represents Rx
ring) for XDP path and for ZC the pointer to xdp_buff that you get from
xsk_buff_pool ? This would satisfy both sides I believe and would let us
keep the same container struct.

struct mlx4_xdp_buff {
	struct xdp_buff *xdp;
	struct mlx4_cqe *cqe;
	struct mlx4_en_dev *mdev;
	struct mlx4_en_rx_ring *ring;
	struct net_device *dev;
};

(...)

	struct mlx4_xdp_buff mxbuf;
	struct xdp_buff xdp;

	mxbuf.xdp = &xdp;
	xdp_init_buff(mxbuf.xdp, priv->frag_info[0].frag_stride, &ring->xdp_rxq);

Also these additional things

+			mxbuf.cqe = cqe;
+			mxbuf.mdev = priv->mdev;
+			mxbuf.ring = ring;
+			mxbuf.dev = dev;

could be assigned once at a setup time or in worse case once per NAPI. So
maybe mlx4_xdp_buff shouldn't be stack based?