XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: Stanislav Fomichev <sdf@google.com>
To: Magnus Karlsson <magnus.karlsson@gmail.com>
Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
	haoluo@google.com, jolsa@kernel.org, kuba@kernel.org,
	toke@kernel.org, willemb@google.com, dsahern@kernel.org,
	magnus.karlsson@intel.com, bjorn@kernel.org, hawk@kernel.org,
	netdev@vger.kernel.org, xdp-hints@xdp-project.net
Subject: [xdp-hints] Re: [PATCH bpf-next 1/9] xsk: Support XDP_TX_METADATA_LEN
Date: Tue, 15 Aug 2023 11:21:31 -0700	[thread overview]
Message-ID: <CAKH8qBvLpaAQaUqdRo5Y2VXdCu4DzfzebDVrHTYsaNRfo+uGTg@mail.gmail.com> (raw)
In-Reply-To: <CAJ8uoz0Hmru4XeVUJK=M97CSML2RMQbucLNTVDFPaZQRjJrgkg@mail.gmail.com>

On Tue, Aug 15, 2023 at 5:19 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:
>
> On Tue, 15 Aug 2023 at 00:25, Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On Mon, Aug 14, 2023 at 11:05 AM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > On Mon, Aug 14, 2023 at 3:57 AM Maciej Fijalkowski
> > > <maciej.fijalkowski@intel.com> wrote:
> > > >
> > > > On Wed, Aug 09, 2023 at 09:54:10AM -0700, Stanislav Fomichev wrote:
> > > > > For zerocopy mode, tx_desc->addr can point to the arbitrary offset
> > > > > and carry some TX metadata in the headroom. For copy mode, there
> > > > > is no way currently to populate skb metadata.
> > > > >
> > > > > Introduce new XDP_TX_METADATA_LEN that indicates how many bytes
> > > > > to treat as metadata. Metadata bytes come prior to tx_desc address
> > > > > (same as in RX case).
> > > > >
> > > > > The size of the metadata has the same constraints as XDP:
> > > > > - less than 256 bytes
> > > > > - 4-byte aligned
> > > > > - non-zero
> > > > >
> > > > > This data is not interpreted in any way right now.
> > > > >
> > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > > ---
> > > > >  include/net/xdp_sock.h      |  1 +
> > > > >  include/net/xsk_buff_pool.h |  1 +
> > > > >  include/uapi/linux/if_xdp.h |  1 +
> > > > >  net/xdp/xsk.c               | 20 ++++++++++++++++++++
> > > > >  net/xdp/xsk_buff_pool.c     |  1 +
> > > > >  net/xdp/xsk_queue.h         | 17 ++++++++++-------
> > > > >  6 files changed, 34 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > > > > index 1617af380162..467b9fb56827 100644
> > > > > --- a/include/net/xdp_sock.h
> > > > > +++ b/include/net/xdp_sock.h
> > > > > @@ -51,6 +51,7 @@ struct xdp_sock {
> > > > >       struct list_head flush_node;
> > > > >       struct xsk_buff_pool *pool;
> > > > >       u16 queue_id;
> > > > > +     u8 tx_metadata_len;
> > > > >       bool zc;
> > > > >       bool sg;
> > > > >       enum {
> > > > > diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> > > > > index b0bdff26fc88..9c31e8d1e198 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_size;
> > > > >       u32 chunk_shift;
> > > > >       u32 frame_len;
> > > > > +     u8 tx_metadata_len; /* inherited from xsk_sock */
> > > > >       u8 cached_need_wakeup;
> > > > >       bool uses_need_wakeup;
> > > > >       bool dma_need_sync;
> > > > > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
> > > > > index 8d48863472b9..b37b50102e1c 100644
> > > > > --- a/include/uapi/linux/if_xdp.h
> > > > > +++ b/include/uapi/linux/if_xdp.h
> > > > > @@ -69,6 +69,7 @@ struct xdp_mmap_offsets {
> > > > >  #define XDP_UMEM_COMPLETION_RING     6
> > > > >  #define XDP_STATISTICS                       7
> > > > >  #define XDP_OPTIONS                  8
> > > > > +#define XDP_TX_METADATA_LEN          9
> > > > >
> > > > >  struct xdp_umem_reg {
> > > > >       __u64 addr; /* Start of packet data area */
> > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > > > index 47796a5a79b3..28df3280501d 100644
> > > > > --- a/net/xdp/xsk.c
> > > > > +++ b/net/xdp/xsk.c
> > > > > @@ -1338,6 +1338,26 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
> > > > >               mutex_unlock(&xs->mutex);
> > > > >               return err;
> > > > >       }
> > > > > +     case XDP_TX_METADATA_LEN:
> > > > > +     {
> > > > > +             int val;
> > > > > +
> > > > > +             if (optlen < sizeof(val))
> > > > > +                     return -EINVAL;
> > > > > +             if (copy_from_sockptr(&val, optval, sizeof(val)))
> > > > > +                     return -EFAULT;
> > > > > +             if (!val || val > 256 || val % 4)
> > > > > +                     return -EINVAL;
> > > > > +
> > > > > +             mutex_lock(&xs->mutex);
> > > > > +             if (xs->state != XSK_READY) {
> > > > > +                     mutex_unlock(&xs->mutex);
> > > > > +                     return -EBUSY;
> > > > > +             }
> > > > > +             xs->tx_metadata_len = val;
> > > > > +             mutex_unlock(&xs->mutex);
> > > > > +             return 0;
> > > > > +     }
> > > > >       default:
> > > > >               break;
> > > > >       }
> > > > > diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> > > > > index b3f7b310811e..b351732f1032 100644
> > > > > --- a/net/xdp/xsk_buff_pool.c
> > > > > +++ b/net/xdp/xsk_buff_pool.c
> > > > > @@ -85,6 +85,7 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
> > > > >               XDP_PACKET_HEADROOM;
> > > > >       pool->umem = umem;
> > > > >       pool->addrs = umem->addrs;
> > > > > +     pool->tx_metadata_len = xs->tx_metadata_len;
> > > >
> > > > Hey Stan,
> > > >
> > > > what would happen in case when one socket sets pool->tx_metadata_len say
> > > > to 16 and the other one that is sharing the pool to 24? If sockets should
> > > > and can have different padding, then this will not work unless the
> > > > metadata_len is on a per socket level.
> > >
> > > Hmm, good point. I didn't think about umem sharing :-/
> > > Maybe they all have to agree on the size? And once the size has been
> > > size by one socket, the same size should be set on the others? (or at
> > > least be implied that the other sockets will use the same metadata
> > > layout)
> >
> > Thinking about it a bit more: should that tx_metadata_len be part of a
> > umem then?
> > Users can provide it as part of xdp_umem_reg which is probably a
> > better place to pass it compared to the setsockopt?
>
> If all the sockets sharing the umem have to agree on the
> tx_metadata_len, then this is a better place than the setsockopt.
> IMHO, it sounds unlikely that multiple versions of the same program,
> or different programs, would like to share the same umem.
>
> Please note that some of the members of struct xdp_umem are copied out
> to the individual struct xsk_buff_pool even though they are the same
> between all of them (chunk_size and the size of the umem for example).
> The reason for this is performance, as to avoid having to access the
> umem struct in the fast path. Something similar might be a good idea
> here too, even though tx_metadata_len is fixed for a umem being
> shared. Might be worth trying.
>
> Again, thanks for working on this Stanislav.

Perfect, thank you, I was trying a similar idea with a copy into the pool!

  reply	other threads:[~2023-08-15 18:21 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-09 16:54 [xdp-hints] [PATCH bpf-next 0/9] xsk: TX metadata Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 1/9] xsk: Support XDP_TX_METADATA_LEN Stanislav Fomichev
2023-08-14 10:56   ` [xdp-hints] " Maciej Fijalkowski
2023-08-14 18:05     ` Stanislav Fomichev
2023-08-14 22:24       ` Stanislav Fomichev
2023-08-15 12:19         ` Magnus Karlsson
2023-08-15 18:21           ` Stanislav Fomichev [this message]
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 2/9] xsk: add TX timestamp and TX checksum offload support Stanislav Fomichev
2023-08-09 20:18   ` [xdp-hints] " Jesper Dangaard Brouer
2023-08-10 18:25     ` Stanislav Fomichev
2023-08-10  5:26   ` kernel test robot
2023-08-10  6:12   ` kernel test robot
2023-08-14 11:01   ` Maciej Fijalkowski
2023-08-14 18:05     ` Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 3/9] tools: ynl: print xsk-features from the sample Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 4/9] net/mlx5e: Implement AF_XDP TX timestamp and checksum offload Stanislav Fomichev
2023-08-14 11:02   ` [xdp-hints] " Maciej Fijalkowski
2023-08-14 18:05     ` Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 5/9] selftests/xsk: Support XDP_TX_METADATA_LEN Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 6/9] selftests/bpf: Add csum helpers Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 7/9] selftests/bpf: Add TX side to xdp_metadata Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 8/9] selftests/bpf: Add TX side to xdp_hw_metadata Stanislav Fomichev
2023-08-09 16:54 ` [xdp-hints] [PATCH bpf-next 9/9] xsk: document XDP_TX_METADATA_LEN layout Stanislav Fomichev
2023-08-09 20:39   ` [xdp-hints] " Jesper Dangaard Brouer
2023-08-10 18:17     ` Stanislav Fomichev
2023-08-09 20:09 ` [xdp-hints] Re: [PATCH bpf-next 0/9] xsk: TX metadata Jesper Dangaard Brouer
2023-08-10 18:23   ` Stanislav Fomichev
2023-08-14 11:13 ` Maciej Fijalkowski
2023-08-14 18:04   ` Stanislav Fomichev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKH8qBvLpaAQaUqdRo5Y2VXdCu4DzfzebDVrHTYsaNRfo+uGTg@mail.gmail.com \
    --to=sdf@google.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dsahern@kernel.org \
    --cc=haoluo@google.com \
    --cc=hawk@kernel.org \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=maciej.fijalkowski@intel.com \
    --cc=magnus.karlsson@gmail.com \
    --cc=magnus.karlsson@intel.com \
    --cc=martin.lau@linux.dev \
    --cc=netdev@vger.kernel.org \
    --cc=song@kernel.org \
    --cc=toke@kernel.org \
    --cc=willemb@google.com \
    --cc=xdp-hints@xdp-project.net \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox