drivers/net/tap.c | 6 +++--- drivers/net/tun.c | 12 +++++------ include/net/xdp.h | 55 +++++++++++++++++++++++++++++++++++++++++++---- net/core/xdp.c | 12 +++++------ 4 files changed, 66 insertions(+), 19 deletions(-)
Introduce new XDP helpers:
- xdp_headlen: Similar to skb_headlen
- xdp_headroom: Similar to skb_headroom
- xdp_metadata_len: Similar to skb_metadata_len
Integrate these helpers into tap, tun, and XDP implementation to start.
No functional changes introduced.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jon Kohler <jon@nutanix.com>
---
v2->v3: Integrate feedback from Stanislav
https://patchwork.kernel.org/project/netdevbpf/patch/20250430201120.1794658-1-jon@nutanix.com/
v1->v2: Integrate feedback from Willem
https://patchwork.kernel.org/project/netdevbpf/patch/20250430182921.1704021-1-jon@nutanix.com/
drivers/net/tap.c | 6 +++---
drivers/net/tun.c | 12 +++++------
include/net/xdp.h | 55 +++++++++++++++++++++++++++++++++++++++++++----
net/core/xdp.c | 12 +++++------
4 files changed, 66 insertions(+), 19 deletions(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index d4ece538f1b2..a62fbca4b08f 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1048,7 +1048,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
struct sk_buff *skb;
int err, depth;
- if (unlikely(xdp->data_end - xdp->data < ETH_HLEN)) {
+ if (unlikely(xdp_headlen(xdp) < ETH_HLEN)) {
err = -EINVAL;
goto err;
}
@@ -1062,8 +1062,8 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
goto err;
}
- skb_reserve(skb, xdp->data - xdp->data_hard_start);
- skb_put(skb, xdp->data_end - xdp->data);
+ skb_reserve(skb, xdp_headroom(xdp));
+ skb_put(skb, xdp_headlen(xdp));
skb_set_network_header(skb, ETH_HLEN);
skb_reset_mac_header(skb);
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 7babd1e9a378..4c47eed71986 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1567,7 +1567,7 @@ static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog,
dev_core_stats_rx_dropped_inc(tun->dev);
return err;
}
- dev_sw_netstats_rx_add(tun->dev, xdp->data_end - xdp->data);
+ dev_sw_netstats_rx_add(tun->dev, xdp_headlen(xdp));
break;
case XDP_TX:
err = tun_xdp_tx(tun->dev, xdp);
@@ -1575,7 +1575,7 @@ static int tun_xdp_act(struct tun_struct *tun, struct bpf_prog *xdp_prog,
dev_core_stats_rx_dropped_inc(tun->dev);
return err;
}
- dev_sw_netstats_rx_add(tun->dev, xdp->data_end - xdp->data);
+ dev_sw_netstats_rx_add(tun->dev, xdp_headlen(xdp));
break;
case XDP_PASS:
break;
@@ -2355,7 +2355,7 @@ static int tun_xdp_one(struct tun_struct *tun,
struct xdp_buff *xdp, int *flush,
struct tun_page *tpage)
{
- unsigned int datasize = xdp->data_end - xdp->data;
+ unsigned int datasize = xdp_headlen(xdp);
struct tun_xdp_hdr *hdr = xdp->data_hard_start;
struct virtio_net_hdr *gso = &hdr->gso;
struct bpf_prog *xdp_prog;
@@ -2415,14 +2415,14 @@ static int tun_xdp_one(struct tun_struct *tun,
goto out;
}
- skb_reserve(skb, xdp->data - xdp->data_hard_start);
- skb_put(skb, xdp->data_end - xdp->data);
+ skb_reserve(skb, xdp_headroom(xdp));
+ skb_put(skb, xdp_headlen(xdp));
/* The externally provided xdp_buff may have no metadata support, which
* is marked by xdp->data_meta being xdp->data + 1. This will lead to a
* metasize of -1 and is the reason why the condition checks for > 0.
*/
- metasize = xdp->data - xdp->data_meta;
+ metasize = xdp_metadata_len(xdp);
if (metasize > 0)
skb_metadata_set(skb, metasize);
diff --git a/include/net/xdp.h b/include/net/xdp.h
index 48efacbaa35d..04c187680f3e 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -151,10 +151,57 @@ xdp_get_shared_info_from_buff(const struct xdp_buff *xdp)
return (struct skb_shared_info *)xdp_data_hard_end(xdp);
}
+/**
+ * xdp_headlen - Calculate the length of the data in an XDP buffer
+ * @xdp: Pointer to the XDP buffer structure
+ *
+ * Compute the length of the data contained in the XDP buffer. Does not
+ * include frags, use xdp_get_buff_len() for that instead.
+ *
+ * Analogous to skb_headlen().
+ *
+ * Return: The length of the data in the XDP buffer in bytes.
+ */
+static inline unsigned int xdp_headlen(const struct xdp_buff *xdp)
+{
+ return xdp->data_end - xdp->data;
+}
+
+/**
+ * xdp_headroom - Calculate the headroom available in an XDP buffer
+ * @xdp: Pointer to the XDP buffer structure
+ *
+ * Compute the headroom in an XDP buffer.
+ *
+ * Analogous to the skb_headroom().
+ *
+ * Return: The size of the headroom in bytes.
+ */
+static inline unsigned int xdp_headroom(const struct xdp_buff *xdp)
+{
+ return xdp->data - xdp->data_hard_start;
+}
+
+/**
+ * xdp_metadata_len - Calculate the length of metadata in an XDP buffer
+ * @xdp: Pointer to the XDP buffer structure
+ *
+ * Compute the length of the metadata region in an XDP buffer.
+ *
+ * Analogous to skb_metadata_len(), though using signed int as value
+ * is allowed to be negative.
+ *
+ * Return: The length of the metadata in bytes.
+ */
+static inline int xdp_metadata_len(const struct xdp_buff *xdp)
+{
+ return xdp->data - xdp->data_meta;
+}
+
static __always_inline unsigned int
xdp_get_buff_len(const struct xdp_buff *xdp)
{
- unsigned int len = xdp->data_end - xdp->data;
+ unsigned int len = xdp_headlen(xdp);
const struct skb_shared_info *sinfo;
if (likely(!xdp_buff_has_frags(xdp)))
@@ -364,8 +411,8 @@ int xdp_update_frame_from_buff(const struct xdp_buff *xdp,
int metasize, headroom;
/* Assure headroom is available for storing info */
- headroom = xdp->data - xdp->data_hard_start;
- metasize = xdp->data - xdp->data_meta;
+ headroom = xdp_headroom(xdp);
+ metasize = xdp_metadata_len(xdp);
metasize = metasize > 0 ? metasize : 0;
if (unlikely((headroom - metasize) < sizeof(*xdp_frame)))
return -ENOSPC;
@@ -377,7 +424,7 @@ int xdp_update_frame_from_buff(const struct xdp_buff *xdp,
}
xdp_frame->data = xdp->data;
- xdp_frame->len = xdp->data_end - xdp->data;
+ xdp_frame->len = xdp_headlen(xdp);
xdp_frame->headroom = headroom - sizeof(*xdp_frame);
xdp_frame->metasize = metasize;
xdp_frame->frame_sz = xdp->frame_sz;
diff --git a/net/core/xdp.c b/net/core/xdp.c
index f86eedad586a..0d56320a7ff9 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -581,8 +581,8 @@ struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp)
/* Clone into a MEM_TYPE_PAGE_ORDER0 xdp_frame. */
metasize = xdp_data_meta_unsupported(xdp) ? 0 :
- xdp->data - xdp->data_meta;
- totsize = xdp->data_end - xdp->data + metasize;
+ xdp_metadata_len(xdp);
+ totsize = xdp_headlen(xdp) + metasize;
if (sizeof(*xdpf) + totsize > PAGE_SIZE)
return NULL;
@@ -646,10 +646,10 @@ struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp)
if (unlikely(!skb))
return NULL;
- skb_reserve(skb, xdp->data - xdp->data_hard_start);
- __skb_put(skb, xdp->data_end - xdp->data);
+ skb_reserve(skb, xdp_headroom(xdp));
+ __skb_put(skb, xdp_headlen(xdp));
- metalen = xdp->data - xdp->data_meta;
+ metalen = xdp_metadata_len(xdp);
if (metalen > 0)
skb_metadata_set(skb, metalen);
@@ -763,7 +763,7 @@ struct sk_buff *xdp_build_skb_from_zc(struct xdp_buff *xdp)
memcpy(__skb_put(skb, len), xdp->data_meta, LARGEST_ALIGN(len));
- metalen = xdp->data - xdp->data_meta;
+ metalen = xdp_metadata_len(xdp);
if (metalen > 0) {
skb_metadata_set(skb, metalen);
__skb_pull(skb, metalen);
--
2.43.0On 5/6/25 2:52 PM, Jon Kohler wrote: > Introduce new XDP helpers: > - xdp_headlen: Similar to skb_headlen > - xdp_headroom: Similar to skb_headroom > - xdp_metadata_len: Similar to skb_metadata_len > > Integrate these helpers into tap, tun, and XDP implementation to start. > > No functional changes introduced. > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Jon Kohler <jon@nutanix.com> lgtm as well, thanks! Acked-by: Daniel Borkmann <daniel@iogearbox.net>
On 05/06, Jon Kohler wrote: > Introduce new XDP helpers: > - xdp_headlen: Similar to skb_headlen > - xdp_headroom: Similar to skb_headroom > - xdp_metadata_len: Similar to skb_metadata_len > > Integrate these helpers into tap, tun, and XDP implementation to start. > > No functional changes introduced. > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Jon Kohler <jon@nutanix.com> > --- > v2->v3: Integrate feedback from Stanislav > https://patchwork.kernel.org/project/netdevbpf/patch/20250430201120.1794658-1-jon@nutanix.com/ Acked-by: Stanislav Fomichev <sdf@fomichev.me> Thanks!
Stanislav Fomichev wrote: > On 05/06, Jon Kohler wrote: > > Introduce new XDP helpers: > > - xdp_headlen: Similar to skb_headlen > > - xdp_headroom: Similar to skb_headroom > > - xdp_metadata_len: Similar to skb_metadata_len > > > > Integrate these helpers into tap, tun, and XDP implementation to start. > > > > No functional changes introduced. > > > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > > Signed-off-by: Jon Kohler <jon@nutanix.com> > > --- > > v2->v3: Integrate feedback from Stanislav > > https://patchwork.kernel.org/project/netdevbpf/patch/20250430201120.1794658-1-jon@nutanix.com/ > > Acked-by: Stanislav Fomichev <sdf@fomichev.me> Reviewed-by: Willem de Bruijn <willemb@google.com>
On 07/05/2025 15.29, Willem de Bruijn wrote: > Stanislav Fomichev wrote: >> On 05/06, Jon Kohler wrote: >>> Introduce new XDP helpers: >>> - xdp_headlen: Similar to skb_headlen I really dislike xdp_headlen(). This "headlen" originates from an SKB implementation detail, that I don't think we should carry over into XDP land. We need to come up with something that isn't easily mis-read as the header-length. >>> - xdp_headroom: Similar to skb_headroom >>> - xdp_metadata_len: Similar to skb_metadata_len >>> I like naming of these. >>> Integrate these helpers into tap, tun, and XDP implementation to start. >>> >>> No functional changes introduced. >>> >>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> >>> Signed-off-by: Jon Kohler <jon@nutanix.com> >>> --- >>> v2->v3: Integrate feedback from Stanislav >>> https://patchwork.kernel.org/project/netdevbpf/patch/20250430201120.1794658-1-jon@nutanix.com/ >> >> Acked-by: Stanislav Fomichev <sdf@fomichev.me> > > Reviewed-by: Willem de Bruijn <willemb@google.com> > Nacked-by: Jesper Dangaard Brouer <hawk@kernel.org> pw: cr
On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: > > > > On 07/05/2025 15.29, Willem de Bruijn wrote: > > Stanislav Fomichev wrote: > >> On 05/06, Jon Kohler wrote: > >>> Introduce new XDP helpers: > >>> - xdp_headlen: Similar to skb_headlen > > I really dislike xdp_headlen(). This "headlen" originates from an SKB > implementation detail, that I don't think we should carry over into XDP > land. > We need to come up with something that isn't easily mis-read as the > header-length. ... snip ... >>> + * xdp_headlen - Calculate the length of the data in an XDP buffer How about xdp_datalen()? On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: > > > > On 07/05/2025 15.29, Willem de Bruijn wrote: > > Stanislav Fomichev wrote: > >> On 05/06, Jon Kohler wrote: > >>> Introduce new XDP helpers: > >>> - xdp_headlen: Similar to skb_headlen > > I really dislike xdp_headlen(). This "headlen" originates from an SKB > implementation detail, that I don't think we should carry over into XDP > land. > We need to come up with something that isn't easily mis-read as the > header-length. > > >>> - xdp_headroom: Similar to skb_headroom > >>> - xdp_metadata_len: Similar to skb_metadata_len > >>> > > I like naming of these. > > >>> Integrate these helpers into tap, tun, and XDP implementation to start. > >>> > >>> No functional changes introduced. > >>> > >>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > >>> Signed-off-by: Jon Kohler <jon@nutanix.com> > >>> --- > >>> v2->v3: Integrate feedback from Stanislav > >>> https://patchwork.kernel.org/project/netdevbpf/patch/20250430201120.1794658-1-jon@nutanix.com/ > >> > >> Acked-by: Stanislav Fomichev <sdf@fomichev.me> > > > > Reviewed-by: Willem de Bruijn <willemb@google.com> > > > > Nacked-by: Jesper Dangaard Brouer <hawk@kernel.org> > > pw: cr >
On 07/05/2025 19.02, Zvi Effron wrote: > On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: >> >> >> >> On 07/05/2025 15.29, Willem de Bruijn wrote: >>> Stanislav Fomichev wrote: >>>> On 05/06, Jon Kohler wrote: >>>>> Introduce new XDP helpers: >>>>> - xdp_headlen: Similar to skb_headlen >> >> I really dislike xdp_headlen(). This "headlen" originates from an SKB >> implementation detail, that I don't think we should carry over into XDP >> land. >> We need to come up with something that isn't easily mis-read as the >> header-length. > > ... snip ... > >>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer > > How about xdp_datalen()? Yes, I like xdp_datalen() :-) --Jesper > On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: >> >> >> >> On 07/05/2025 15.29, Willem de Bruijn wrote: >>> Stanislav Fomichev wrote: >>>> On 05/06, Jon Kohler wrote: >>>>> Introduce new XDP helpers: >>>>> - xdp_headlen: Similar to skb_headlen >> >> I really dislike xdp_headlen(). This "headlen" originates from an SKB >> implementation detail, that I don't think we should carry over into XDP >> land. >> We need to come up with something that isn't easily mis-read as the >> header-length. >> >>>>> - xdp_headroom: Similar to skb_headroom >>>>> - xdp_metadata_len: Similar to skb_metadata_len >>>>> >> >> I like naming of these. >> >>>>> Integrate these helpers into tap, tun, and XDP implementation to start. >>>>> >>>>> No functional changes introduced. >>>>> >>>>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> >>>>> Signed-off-by: Jon Kohler <jon@nutanix.com> >>>>> --- >>>>> v2->v3: Integrate feedback from Stanislav >>>>> https://patchwork.kernel.org/project/netdevbpf/patch/20250430201120.1794658-1-jon@nutanix.com/ >>>> >>>> Acked-by: Stanislav Fomichev <sdf@fomichev.me> >>> >>> Reviewed-by: Willem de Bruijn <willemb@google.com> >>> >> >> Nacked-by: Jesper Dangaard Brouer <hawk@kernel.org> >> >> pw: cr >>
Jesper Dangaard Brouer wrote: > > > On 07/05/2025 19.02, Zvi Effron wrote: > > On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: > >> > >> > >> > >> On 07/05/2025 15.29, Willem de Bruijn wrote: > >>> Stanislav Fomichev wrote: > >>>> On 05/06, Jon Kohler wrote: > >>>>> Introduce new XDP helpers: > >>>>> - xdp_headlen: Similar to skb_headlen > >> > >> I really dislike xdp_headlen(). This "headlen" originates from an SKB > >> implementation detail, that I don't think we should carry over into XDP > >> land. > >> We need to come up with something that isn't easily mis-read as the > >> header-length. > > > > ... snip ... > > > >>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer > > > > How about xdp_datalen()? > > Yes, I like xdp_datalen() :-) This is confusing in that it is the inverse of skb->data_len: which is exactly the part of the data not in the skb head. There is value in consistent naming. I've never confused headlen with header len. But if diverging, at least let's choose something not associated with skbs with a different meaning.
> On May 7, 2025, at 1:21 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > Jesper Dangaard Brouer wrote: >> >> >> On 07/05/2025 19.02, Zvi Effron wrote: >>> On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: >>>> >>>> >>>> >>>> On 07/05/2025 15.29, Willem de Bruijn wrote: >>>>> Stanislav Fomichev wrote: >>>>>> On 05/06, Jon Kohler wrote: >>>>>>> Introduce new XDP helpers: >>>>>>> - xdp_headlen: Similar to skb_headlen >>>> >>>> I really dislike xdp_headlen(). This "headlen" originates from an SKB >>>> implementation detail, that I don't think we should carry over into XDP >>>> land. >>>> We need to come up with something that isn't easily mis-read as the >>>> header-length. >>> >>> ... snip ... >>> >>>>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer >>> >>> How about xdp_datalen()? >> >> Yes, I like xdp_datalen() :-) > > This is confusing in that it is the inverse of skb->data_len: > which is exactly the part of the data not in the skb head. > > There is value in consistent naming. I've never confused headlen > with header len. > > But if diverging, at least let's choose something not > associated with skbs with a different meaning. Brainstorming a few options: - xdp_head_datalen() ? - xdp_base_datalen() ? - xdp_base_headlen() ? - xdp_buff_datalen() ? - xdp_buff_headlen() ? - xdp_datalen() ? (ZivE, JesperB) - xdp_headlen() ? (WillemB, JonK, StanislavF, JacobK, DanielB)
On 07/05/2025 19.47, Jon Kohler wrote: > > >> On May 7, 2025, at 1:21 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: >> >> >> Jesper Dangaard Brouer wrote: >>> >>> >>> On 07/05/2025 19.02, Zvi Effron wrote: >>>> On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: >>>>> >>>>> >>>>> >>>>> On 07/05/2025 15.29, Willem de Bruijn wrote: >>>>>> Stanislav Fomichev wrote: >>>>>>> On 05/06, Jon Kohler wrote: >>>>>>>> Introduce new XDP helpers: >>>>>>>> - xdp_headlen: Similar to skb_headlen >>>>> >>>>> I really dislike xdp_headlen(). This "headlen" originates from an SKB >>>>> implementation detail, that I don't think we should carry over into XDP >>>>> land. >>>>> We need to come up with something that isn't easily mis-read as the >>>>> header-length. >>>> >>>> ... snip ... >>>> >>>>>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer >>>> >>>> How about xdp_datalen()? >>> >>> Yes, I like xdp_datalen() :-) >> >> This is confusing in that it is the inverse of skb->data_len: >> which is exactly the part of the data not in the skb head. >> >> There is value in consistent naming. I've never confused headlen >> with header len. >> >> But if diverging, at least let's choose something not >> associated with skbs with a different meaning. > > Brainstorming a few options: > - xdp_head_datalen() ? > - xdp_base_datalen() ? > - xdp_base_headlen() ? > - xdp_buff_datalen() ? > - xdp_buff_headlen() ? > - xdp_datalen() ? (ZivE, JesperB) > - xdp_headlen() ? (WillemB, JonK, StanislavF, JacobK, DanielB) > What about keeping it really simple: xdp_buff_len() ? Or even simpler: xdp_len() as the function documentation already describe this doesn't include frags. To Jon, you seems to be on a cleanup spree: For SKBs netstack have this diagram documented [1]. Which also explains the concept of a "head" buffer, which isn't a concept for XDP. I would really like to see a diagram documenting both xdp_buff and xdp_frame data structures via ascii art, like the one for SKBs. (Hint, this is actually defined in the header file include/linux/skbuff.h, but converted to RST/HTML format.) [1] https://docs.kernel.org/networking/skbuff.html --Jesper
> On May 7, 2025, at 3:04 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote: > > > > On 07/05/2025 19.47, Jon Kohler wrote: >>> On May 7, 2025, at 1:21 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: >>> >>> >>> Jesper Dangaard Brouer wrote: >>>> >>>> >>>> On 07/05/2025 19.02, Zvi Effron wrote: >>>>> On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 07/05/2025 15.29, Willem de Bruijn wrote: >>>>>>> Stanislav Fomichev wrote: >>>>>>>> On 05/06, Jon Kohler wrote: >>>>>>>>> Introduce new XDP helpers: >>>>>>>>> - xdp_headlen: Similar to skb_headlen >>>>>> >>>>>> I really dislike xdp_headlen(). This "headlen" originates from an SKB >>>>>> implementation detail, that I don't think we should carry over into XDP >>>>>> land. >>>>>> We need to come up with something that isn't easily mis-read as the >>>>>> header-length. >>>>> >>>>> ... snip ... >>>>> >>>>>>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer >>>>> >>>>> How about xdp_datalen()? >>>> >>>> Yes, I like xdp_datalen() :-) >>> >>> This is confusing in that it is the inverse of skb->data_len: >>> which is exactly the part of the data not in the skb head. >>> >>> There is value in consistent naming. I've never confused headlen >>> with header len. >>> >>> But if diverging, at least let's choose something not >>> associated with skbs with a different meaning. >> Brainstorming a few options: >> - xdp_head_datalen() ? >> - xdp_base_datalen() ? >> - xdp_base_headlen() ? >> - xdp_buff_datalen() ? >> - xdp_buff_headlen() ? >> - xdp_datalen() ? (ZivE, JesperB) >> - xdp_headlen() ? (WillemB, JonK, StanislavF, JacobK, DanielB) > > What about keeping it really simple: xdp_buff_len() ? This is suspiciously close to xdp_get_buff_len(), so there could be some confusion there, since that takes paged/frags into account transparently. > > Or even simpler: xdp_len() as the function documentation already > describe this doesn't include frags. There is a neat hint from Lorenzo’s change in bpf.h for bpf_xdp_get_buff_len() that talks about both linear and paged length. Also, xdp_buff_flags’s XDP_FLAGS_HAS_FRAGS says non-linear xdp buff. Taking those hints, what about: xdp_linear_len() == xdp->data_end - xdp->data xdp_paged_len() == sinfo->xdp_frags_size xdp_get_buff_len() == xdp_linear_len() + xdp_paged_len() Just a thought. If not, that’s ok. I’m happy to do xdp_len, but do you then have a suggestion about getting the non-linear size only? > > To Jon, you seems to be on a cleanup spree: > For SKBs netstack have this diagram documented [1]. Which also explains > the concept of a "head" buffer, which isn't a concept for XDP. I would > really like to see a diagram documenting both xdp_buff and xdp_frame > data structures via ascii art, like the one for SKBs. (Hint, this is > actually defined in the header file include/linux/skbuff.h, but > converted to RST/HTML format.) > > [1] https://docs.kernel.org/networking/skbuff.html I certainly am in a cleanup sort of mood, happy to help here. I see what you're talking about, I’ll take a stab at this in a separate patch. Thanks for the push and tip!
On 07/05/2025 21.57, Jon Kohler wrote:
>
>
>> On May 7, 2025, at 3:04 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>>
>>
>>
>> On 07/05/2025 19.47, Jon Kohler wrote:
>>>> On May 7, 2025, at 1:21 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>>>>
>>>>
>>>> Jesper Dangaard Brouer wrote:
>>>>>
>>>>>
>>>>> On 07/05/2025 19.02, Zvi Effron wrote:
>>>>>> On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 07/05/2025 15.29, Willem de Bruijn wrote:
>>>>>>>> Stanislav Fomichev wrote:
>>>>>>>>> On 05/06, Jon Kohler wrote:
>>>>>>>>>> Introduce new XDP helpers:
>>>>>>>>>> - xdp_headlen: Similar to skb_headlen
>>>>>>>
>>>>>>> I really dislike xdp_headlen(). This "headlen" originates from an SKB
>>>>>>> implementation detail, that I don't think we should carry over into XDP
>>>>>>> land.
>>>>>>> We need to come up with something that isn't easily mis-read as the
>>>>>>> header-length.
>>>>>>
>>>>>> ... snip ...
>>>>>>
>>>>>>>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer
>>>>>>
>>>>>> How about xdp_datalen()?
>>>>>
>>>>> Yes, I like xdp_datalen() :-)
>>>>
>>>> This is confusing in that it is the inverse of skb->data_len:
>>>> which is exactly the part of the data not in the skb head.
>>>>
>>>> There is value in consistent naming. I've never confused headlen
>>>> with header len.
>>>>
>>>> But if diverging, at least let's choose something not
>>>> associated with skbs with a different meaning.
>>> Brainstorming a few options:
>>> - xdp_head_datalen() ?
>>> - xdp_base_datalen() ?
>>> - xdp_base_headlen() ?
>>> - xdp_buff_datalen() ?
>>> - xdp_buff_headlen() ?
>>> - xdp_datalen() ? (ZivE, JesperB)
>>> - xdp_headlen() ? (WillemB, JonK, StanislavF, JacobK, DanielB)
>>
>> What about keeping it really simple: xdp_buff_len() ?
>
> This is suspiciously close to xdp_get_buff_len(), so there could be some
> confusion there, since that takes paged/frags into account transparently.
Good point.
>>
>> Or even simpler: xdp_len() as the function documentation already
>> describe this doesn't include frags.
>
> There is a neat hint from Lorenzo’s change in bpf.h for bpf_xdp_get_buff_len()
> that talks about both linear and paged length. Also, xdp_buff_flags’s
> XDP_FLAGS_HAS_FRAGS says non-linear xdp buff.
>
> Taking those hints, what about:
> xdp_linear_len() == xdp->data_end - xdp->data
> xdp_paged_len() == sinfo->xdp_frags_size
> xdp_get_buff_len() == xdp_linear_len() + xdp_paged_len()
>
I like xdp_linear_len() as it is descriptive/clear.
> Just a thought. If not, that’s ok. I’m happy to do xdp_len, but do you then have a
> suggestion about getting the non-linear size only?
>
I've not checked if we have API users that need to get the non-linear
size only...
A history rant:
XDP started out as being limited to one-page ("packet-pages" was my
original bad name). With a fixed XDP_HEADROOM of 256 bytes and reserved
tailroom of 320 bytes sizeof(skb_shared_info) to be compatible with
creating an SKB (that can use this as a "head" page). Limiting max MTU
to be 3502 (assuming Eth(14)+2 VLAN headers=18).
These constraints were why XDP was so fast. As time goes on we continue
to add features and performance paper-cuts. Today, XDP_HEADROOM have
become variable, leading to checks all over. With XDP multi-buffer
support getting more features, we also have to add check all over for that.
WARNING to end-users: XDP programs that use xdp.frags and the associated
helpers are really SLOW (as these helper need to copy out data to stack
or elsewhere). XDP is only fast if your XDP prog read the linear path
with the older helpers (direct access) and ignore if packet have frags.
We are slowly but surely making XDP slower and slower by paper-cuts.
Guess, we should clearly document that, such that people don't think XDP
multi-buffer access is fast. Sorry for the rant.
>>
>> To Jon, you seems to be on a cleanup spree:
>> For SKBs netstack have this diagram documented [1]. Which also explains
>> the concept of a "head" buffer, which isn't a concept for XDP. I would
>> really like to see a diagram documenting both xdp_buff and xdp_frame
>> data structures via ascii art, like the one for SKBs. (Hint, this is
>> actually defined in the header file include/linux/skbuff.h, but
>> converted to RST/HTML format.)
>>
>> [1] https://docs.kernel.org/networking/skbuff.html
>
> I certainly am in a cleanup sort of mood, happy to help here. I see what
> you're talking about, I’ll take a stab at this in a separate patch. Thanks
> for the push and tip!
Thanks for the cleanups.
--Jesper
> On May 7, 2025, at 4:58 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>
>
>
> On 07/05/2025 21.57, Jon Kohler wrote:
>>> On May 7, 2025, at 3:04 PM, Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>>>
>>>
>>>
>>> On 07/05/2025 19.47, Jon Kohler wrote:
>>>>> On May 7, 2025, at 1:21 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>>>>>
>>>>>
>>>>> Jesper Dangaard Brouer wrote:
>>>>>>
>>>>>>
>>>>>> On 07/05/2025 19.02, Zvi Effron wrote:
>>>>>>> On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer <hawk@kernel.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 07/05/2025 15.29, Willem de Bruijn wrote:
>>>>>>>>> Stanislav Fomichev wrote:
>>>>>>>>>> On 05/06, Jon Kohler wrote:
>>>>>>>>>>> Introduce new XDP helpers:
>>>>>>>>>>> - xdp_headlen: Similar to skb_headlen
>>>>>>>>
>>>>>>>> I really dislike xdp_headlen(). This "headlen" originates from an SKB
>>>>>>>> implementation detail, that I don't think we should carry over into XDP
>>>>>>>> land.
>>>>>>>> We need to come up with something that isn't easily mis-read as the
>>>>>>>> header-length.
>>>>>>>
>>>>>>> ... snip ...
>>>>>>>
>>>>>>>>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer
>>>>>>>
>>>>>>> How about xdp_datalen()?
>>>>>>
>>>>>> Yes, I like xdp_datalen() :-)
>>>>>
>>>>> This is confusing in that it is the inverse of skb->data_len:
>>>>> which is exactly the part of the data not in the skb head.
>>>>>
>>>>> There is value in consistent naming. I've never confused headlen
>>>>> with header len.
>>>>>
>>>>> But if diverging, at least let's choose something not
>>>>> associated with skbs with a different meaning.
>>>> Brainstorming a few options:
>>>> - xdp_head_datalen() ?
>>>> - xdp_base_datalen() ?
>>>> - xdp_base_headlen() ?
>>>> - xdp_buff_datalen() ?
>>>> - xdp_buff_headlen() ?
>>>> - xdp_datalen() ? (ZivE, JesperB)
>>>> - xdp_headlen() ? (WillemB, JonK, StanislavF, JacobK, DanielB)
>>>
>>> What about keeping it really simple: xdp_buff_len() ?
>> This is suspiciously close to xdp_get_buff_len(), so there could be some
>> confusion there, since that takes paged/frags into account transparently.
>
> Good point.
>
>>>
>>> Or even simpler: xdp_len() as the function documentation already
>>> describe this doesn't include frags.
>> There is a neat hint from Lorenzo’s change in bpf.h for bpf_xdp_get_buff_len()
>> that talks about both linear and paged length. Also, xdp_buff_flags’s
>> XDP_FLAGS_HAS_FRAGS says non-linear xdp buff.
>> Taking those hints, what about:
>> xdp_linear_len() == xdp->data_end - xdp->data
>> xdp_paged_len() == sinfo->xdp_frags_size
>> xdp_get_buff_len() == xdp_linear_len() + xdp_paged_len()
>
> I like xdp_linear_len() as it is descriptive/clear.
Ok thanks, I’ll send out a v4 to codify that.
>
>
>> Just a thought. If not, that’s ok. I’m happy to do xdp_len, but do you then have a
>> suggestion about getting the non-linear size only?
>>
>
> I've not checked if we have API users that need to get the non-linear size only...
>
> A history rant:
> XDP started out as being limited to one-page ("packet-pages" was my
> original bad name). With a fixed XDP_HEADROOM of 256 bytes and reserved
> tailroom of 320 bytes sizeof(skb_shared_info) to be compatible with
> creating an SKB (that can use this as a "head" page). Limiting max MTU
> to be 3502 (assuming Eth(14)+2 VLAN headers=18).
> These constraints were why XDP was so fast. As time goes on we continue
> to add features and performance paper-cuts. Today, XDP_HEADROOM have
> become variable, leading to checks all over. With XDP multi-buffer
> support getting more features, we also have to add check all over for that.
> WARNING to end-users: XDP programs that use xdp.frags and the associated
> helpers are really SLOW (as these helper need to copy out data to stack
> or elsewhere). XDP is only fast if your XDP prog read the linear path
> with the older helpers (direct access) and ignore if packet have frags.
> We are slowly but surely making XDP slower and slower by paper-cuts.
> Guess, we should clearly document that, such that people don't think XDP
> multi-buffer access is fast. Sorry for the rant.
No worries on the rant, and I appreciate the context. My multi buffer journey
is exploring the possibility of using that as the mechanism to batch up large
payloads from vhost/net, as small payloads have this slick XDP-based batching
mechanism that then dequeues the batch thru tun_xdp_one, but large GSO
payloads go down a slower, not batched path, and also force all of the small
payloads to flush first.
I have it half-ish working, so I’ll get more excited (or not?) about that later.
>
>
>>>
>>> To Jon, you seems to be on a cleanup spree:
>>> For SKBs netstack have this diagram documented [1]. Which also explains
>>> the concept of a "head" buffer, which isn't a concept for XDP. I would
>>> really like to see a diagram documenting both xdp_buff and xdp_frame
>>> data structures via ascii art, like the one for SKBs. (Hint, this is
>>> actually defined in the header file include/linux/skbuff.h, but
>>> converted to RST/HTML format.)
>>>
>>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.kernel.org_networking_skbuff.html&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=NGPRGGo37mQiSXgHKm5rCQ&m=-gFjIo6lMJVmxoGTrO99FYTlCi0KdthxFuSRv1nasjBI-qJrqdBuQDTN1TrXArrD&s=X16zsPJ_lJwLgJjKJHvKVzMFkuAjEgyZYfDsoCVugyY&e=
>> I certainly am in a cleanup sort of mood, happy to help here. I see what
>> you're talking about, I’ll take a stab at this in a separate patch. Thanks
>> for the push and tip!
>
> Thanks for the cleanups.
> --Jesper
On 08/05/2025 05.18, Jon Kohler wrote: > >> On May 7, 2025, at 4:58 PM, Jesper Dangaard Brouer<hawk@kernel.org> wrote: >> >> On 07/05/2025 21.57, Jon Kohler wrote: >>>> On May 7, 2025, at 3:04 PM, Jesper Dangaard Brouer<hawk@kernel.org> wrote: >>>> >>>> On 07/05/2025 19.47, Jon Kohler wrote: >>>>>> On May 7, 2025, at 1:21 PM, Willem de Bruijn<willemdebruijn.kernel@gmail.com> wrote: >>>>>> >>>>>> Jesper Dangaard Brouer wrote: >>>>>>> >>>>>>> On 07/05/2025 19.02, Zvi Effron wrote: >>>>>>>> On Wed, May 7, 2025 at 9:37 AM Jesper Dangaard Brouer<hawk@kernel.org> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 07/05/2025 15.29, Willem de Bruijn wrote: >>>>>>>>>> Stanislav Fomichev wrote: >>>>>>>>>>> On 05/06, Jon Kohler wrote: >>>>>>>>>>>> Introduce new XDP helpers: >>>>>>>>>>>> - xdp_headlen: Similar to skb_headlen >>>>>>>>> I really dislike xdp_headlen(). This "headlen" originates from an SKB >>>>>>>>> implementation detail, that I don't think we should carry over into XDP >>>>>>>>> land. >>>>>>>>> We need to come up with something that isn't easily mis-read as the >>>>>>>>> header-length. >>>>>>>> ... snip ... >>>>>>>> >>>>>>>>>>> + * xdp_headlen - Calculate the length of the data in an XDP buffer >>>>>>>> How about xdp_datalen()? >>>>>>> Yes, I like xdp_datalen() 🙂 >>>>>> This is confusing in that it is the inverse of skb->data_len: >>>>>> which is exactly the part of the data not in the skb head. >>>>>> >>>>>> There is value in consistent naming. I've never confused headlen >>>>>> with header len. >>>>>> >>>>>> But if diverging, at least let's choose something not >>>>>> associated with skbs with a different meaning. >>>>> Brainstorming a few options: >>>>> - xdp_head_datalen() ? >>>>> - xdp_base_datalen() ? >>>>> - xdp_base_headlen() ? >>>>> - xdp_buff_datalen() ? >>>>> - xdp_buff_headlen() ? >>>>> - xdp_datalen() ? (ZivE, JesperB) >>>>> - xdp_headlen() ? (WillemB, JonK, StanislavF, JacobK, DanielB) >>>> What about keeping it really simple: xdp_buff_len() ? >>> This is suspiciously close to xdp_get_buff_len(), so there could be some >>> confusion there, since that takes paged/frags into account transparently. >> Good point. >> >>>> Or even simpler: xdp_len() as the function documentation already >>>> describe this doesn't include frags. >>> There is a neat hint from Lorenzo’s change in bpf.h for bpf_xdp_get_buff_len() >>> that talks about both linear and paged length. Also, xdp_buff_flags’s >>> XDP_FLAGS_HAS_FRAGS says non-linear xdp buff. >>> Taking those hints, what about: >>> xdp_linear_len() == xdp->data_end - xdp->data >>> xdp_paged_len() == sinfo->xdp_frags_size >>> xdp_get_buff_len() == xdp_linear_len() + xdp_paged_len() >> I like xdp_linear_len() as it is descriptive/clear. > > Ok thanks, I’ll send out a v4 to codify that. I'll ack a V4 with that change. I do notice Jakub isn't a fan of the patch in general, but it seems quite popular given the other high profile kernel developers that acked in V3. I think it increase code readability for people that are less familiar with XDP code and meaning of the pointers (e.g. data_hard_start vs. data_end vs. data vs. data_meta). (We don't even have some ascii art showing these pointers). --Jesper
On Thu, 8 May 2025 18:59:54 +0200 Jesper Dangaard Brouer wrote: > I do notice Jakub isn't a fan of the patch in general, but it seems > quite popular given the other high profile kernel developers that acked > in V3. I think it increase code readability for people that are less > familiar with XDP code and meaning of the pointers (e.g. data_hard_start > vs. data_end vs. data vs. data_meta). (We don't even have some ascii art > showing these pointers). Yup, ASCII art would be great, hopefully DOC: Basic sk_buff geometry can be an inspiration.
On Wed, 7 May 2025 22:58:33 +0200 Jesper Dangaard Brouer wrote: > > There is a neat hint from Lorenzo’s change in bpf.h for bpf_xdp_get_buff_len() > > that talks about both linear and paged length. Also, xdp_buff_flags’s > > XDP_FLAGS_HAS_FRAGS says non-linear xdp buff. > > > > Taking those hints, what about: > > xdp_linear_len() == xdp->data_end - xdp->data > > xdp_paged_len() == sinfo->xdp_frags_size > > xdp_get_buff_len() == xdp_linear_len() + xdp_paged_len() > > I like xdp_linear_len() as it is descriptive/clear. FWIW I don't feel strongly but my very weak preference would be not to merge this. I already know I'll be looking at the definitions every time. Is it obvious to everyone in this thread whether "headroom" includes the metadata length? It's not obvious to me. But the patch seems quite popular so 🤷️
> On May 7, 2025, at 8:18 PM, Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 7 May 2025 22:58:33 +0200 Jesper Dangaard Brouer wrote: >>> There is a neat hint from Lorenzo’s change in bpf.h for bpf_xdp_get_buff_len() >>> that talks about both linear and paged length. Also, xdp_buff_flags’s >>> XDP_FLAGS_HAS_FRAGS says non-linear xdp buff. >>> >>> Taking those hints, what about: >>> xdp_linear_len() == xdp->data_end - xdp->data >>> xdp_paged_len() == sinfo->xdp_frags_size >>> xdp_get_buff_len() == xdp_linear_len() + xdp_paged_len() >> >> I like xdp_linear_len() as it is descriptive/clear. > > FWIW I don't feel strongly but my very weak preference would be > not to merge this. I already know I'll be looking at the definitions > every time. Is it obvious to everyone in this thread whether "headroom" > includes the metadata length? It's not obvious to me. But the patch > seems quite popular so 🤷️ Jespers suggestion to have a DOC: on this hopefully will be helpful. I’ll try my hand that that and see what sort of trouble I can get into
On Thu, 8 May 2025 03:19:42 +0000 Jon Kohler wrote: > >> I like xdp_linear_len() as it is descriptive/clear. > > > > FWIW I don't feel strongly but my very weak preference would be > > not to merge this. I already know I'll be looking at the definitions > > every time. Is it obvious to everyone in this thread whether "headroom" > > includes the metadata length? It's not obvious to me. But the patch > > seems quite popular so 🤷️ > > Jespers suggestion to have a DOC: on this hopefully will be helpful. > > I’ll try my hand that that and see what sort of trouble I can get into I hate to dwell since I already said I don't feel strongly :) but the doc is orthogonal, it helps people who don't understand the geometry to understand it. My concern was that I understand the geometry, but now I will also need to remember what we decided to call "headroom" since (unlike skbs) XDP has all sort of stuff packed in front of the packet data :)
© 2016 - 2025 Red Hat, Inc.