* [PATCH net-next] xdp: add xdp_skb_reserve_put helper
@ 2025-04-30 18:29 Jon Kohler
2025-04-30 18:25 ` Willem de Bruijn
0 siblings, 1 reply; 6+ messages in thread
From: Jon Kohler @ 2025-04-30 18:29 UTC (permalink / raw)
To: Willem de Bruijn, Jason Wang, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
Simon Horman, netdev, linux-kernel, bpf
Cc: Jon Kohler
Add helper for calling skb_{put|reserve} to reduce repetitive pattern
across various drivers.
Plumb into tap and tun to start.
No functional change intended.
Signed-off-by: Jon Kohler <jon@nutanix.com>
---
drivers/net/tap.c | 3 +--
drivers/net/tun.c | 3 +--
include/net/xdp.h | 8 ++++++++
net/core/xdp.c | 3 +--
4 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index d4ece538f1b2..54ce492da5e9 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1062,8 +1062,7 @@ 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);
+ xdp_skb_reserve_put(xdp, skb);
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..30701ad5c27d 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2415,8 +2415,7 @@ 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);
+ xdp_skb_reserve_put(xdp, skb);
/* 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
diff --git a/include/net/xdp.h b/include/net/xdp.h
index 48efacbaa35d..0e7414472464 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -345,6 +345,14 @@ struct sk_buff *xdp_build_skb_from_frame(struct xdp_frame *xdpf,
struct net_device *dev);
struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf);
+static __always_inline
+void xdp_skb_reserve_put(const struct xdp_buff *xdp,
+ struct sk_buff *skb)
+{
+ skb_reserve(skb, xdp->data - xdp->data_hard_start);
+ __skb_put(skb, xdp->data_end - xdp->data);
+}
+
static inline
void xdp_convert_frame_to_buff(const struct xdp_frame *frame,
struct xdp_buff *xdp)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index f86eedad586a..1fca2aa1d1fe 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -646,8 +646,7 @@ 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);
+ xdp_skb_reserve_put(xdp, skb);
metalen = xdp->data - xdp->data_meta;
if (metalen > 0)
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] xdp: add xdp_skb_reserve_put helper
2025-04-30 18:29 [PATCH net-next] xdp: add xdp_skb_reserve_put helper Jon Kohler
@ 2025-04-30 18:25 ` Willem de Bruijn
2025-04-30 18:40 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Willem de Bruijn @ 2025-04-30 18:25 UTC (permalink / raw)
To: Jon Kohler, Willem de Bruijn, Jason Wang, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
John Fastabend, Simon Horman, netdev, linux-kernel, bpf
Cc: Jon Kohler
Jon Kohler wrote:
> Add helper for calling skb_{put|reserve} to reduce repetitive pattern
> across various drivers.
>
> Plumb into tap and tun to start.
>
> No functional change intended.
>
> Signed-off-by: Jon Kohler <jon@nutanix.com>
> ---
> drivers/net/tap.c | 3 +--
> drivers/net/tun.c | 3 +--
> include/net/xdp.h | 8 ++++++++
> net/core/xdp.c | 3 +--
> 4 files changed, 11 insertions(+), 6 deletions(-)
Subjective, but I prefer the existing code. I understand what
skb_reserve and skb_put do. While xdp_skb_reserve_put adds a layer of
indirection that I'd have to follow.
Sometimes deduplication makes sense, sometimes the indirection adds
more mental load than it's worth. In this case the code savings are
small. As said, subjective. Happy to hear other opinions.
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index d4ece538f1b2..54ce492da5e9 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -1062,8 +1062,7 @@ 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);
> + xdp_skb_reserve_put(xdp, skb);
>
> 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..30701ad5c27d 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2415,8 +2415,7 @@ 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);
> + xdp_skb_reserve_put(xdp, skb);
>
> /* 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
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 48efacbaa35d..0e7414472464 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -345,6 +345,14 @@ struct sk_buff *xdp_build_skb_from_frame(struct xdp_frame *xdpf,
> struct net_device *dev);
> struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf);
>
> +static __always_inline
> +void xdp_skb_reserve_put(const struct xdp_buff *xdp,
> + struct sk_buff *skb)
> +{
> + skb_reserve(skb, xdp->data - xdp->data_hard_start);
> + __skb_put(skb, xdp->data_end - xdp->data);
> +}
> +
> static inline
> void xdp_convert_frame_to_buff(const struct xdp_frame *frame,
> struct xdp_buff *xdp)
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index f86eedad586a..1fca2aa1d1fe 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -646,8 +646,7 @@ 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);
> + xdp_skb_reserve_put(xdp, skb);
>
> metalen = xdp->data - xdp->data_meta;
> if (metalen > 0)
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] xdp: add xdp_skb_reserve_put helper
2025-04-30 18:25 ` Willem de Bruijn
@ 2025-04-30 18:40 ` Daniel Borkmann
2025-04-30 18:45 ` Jon Kohler
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2025-04-30 18:40 UTC (permalink / raw)
To: Willem de Bruijn, Jon Kohler, Jason Wang, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Alexei Starovoitov, Jesper Dangaard Brouer, John Fastabend,
Simon Horman, netdev, linux-kernel, bpf
On 4/30/25 8:25 PM, Willem de Bruijn wrote:
> Jon Kohler wrote:
>> Add helper for calling skb_{put|reserve} to reduce repetitive pattern
>> across various drivers.
>>
>> Plumb into tap and tun to start.
>>
>> No functional change intended.
>>
>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>> ---
>> drivers/net/tap.c | 3 +--
>> drivers/net/tun.c | 3 +--
>> include/net/xdp.h | 8 ++++++++
>> net/core/xdp.c | 3 +--
>> 4 files changed, 11 insertions(+), 6 deletions(-)
>
> Subjective, but I prefer the existing code. I understand what
> skb_reserve and skb_put do. While xdp_skb_reserve_put adds a layer of
> indirection that I'd have to follow.
>
> Sometimes deduplication makes sense, sometimes the indirection adds
> more mental load than it's worth. In this case the code savings are
> small. As said, subjective. Happy to hear other opinions.
+1, agree with Willem
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] xdp: add xdp_skb_reserve_put helper
2025-04-30 18:40 ` Daniel Borkmann
@ 2025-04-30 18:45 ` Jon Kohler
2025-04-30 19:04 ` Willem de Bruijn
0 siblings, 1 reply; 6+ messages in thread
From: Jon Kohler @ 2025-04-30 18:45 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Willem de Bruijn, Jason Wang, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Jesper Dangaard Brouer, John Fastabend, Simon Horman,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
bpf@vger.kernel.org
> On Apr 30, 2025, at 2:40 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On 4/30/25 8:25 PM, Willem de Bruijn wrote:
>> Jon Kohler wrote:
>>> Add helper for calling skb_{put|reserve} to reduce repetitive pattern
>>> across various drivers.
>>>
>>> Plumb into tap and tun to start.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>>> ---
>>> drivers/net/tap.c | 3 +--
>>> drivers/net/tun.c | 3 +--
>>> include/net/xdp.h | 8 ++++++++
>>> net/core/xdp.c | 3 +--
>>> 4 files changed, 11 insertions(+), 6 deletions(-)
>> Subjective, but I prefer the existing code. I understand what
>> skb_reserve and skb_put do. While xdp_skb_reserve_put adds a layer of
>> indirection that I'd have to follow.
>> Sometimes deduplication makes sense, sometimes the indirection adds
>> more mental load than it's worth. In this case the code savings are
>> small. As said, subjective. Happy to hear other opinions.
>
> +1, agree with Willem
That’s a fair point. I was also toying with the idea of something like
this instead:
e.g.
xdp_headroom(xdp) == xdp->data - xdp->data_hard_start
… similar to skb_headroom
xdp_length_base(xdp) == xdp->data_end - xdp->data
… similar to xdp_get_buff_len, but doesn’t look at frags
then we could do:
skb_reserve(skb, xdp_headroom(xdp));
skb_put(skb, xdp_length_base(xdp));
Names TBD of course, but thoughts?
That way we keep skb_reserve/put just the same, but have
a nice helper like we do for skb_headroom() already
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] xdp: add xdp_skb_reserve_put helper
2025-04-30 18:45 ` Jon Kohler
@ 2025-04-30 19:04 ` Willem de Bruijn
2025-04-30 19:09 ` Jon Kohler
0 siblings, 1 reply; 6+ messages in thread
From: Willem de Bruijn @ 2025-04-30 19:04 UTC (permalink / raw)
To: Jon Kohler, Daniel Borkmann
Cc: Willem de Bruijn, Jason Wang, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Jesper Dangaard Brouer, John Fastabend, Simon Horman,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
bpf@vger.kernel.org
Jon Kohler wrote:
>
>
> > On Apr 30, 2025, at 2:40 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > !-------------------------------------------------------------------|
> > CAUTION: External Email
> >
> > |-------------------------------------------------------------------!
> >
> > On 4/30/25 8:25 PM, Willem de Bruijn wrote:
> >> Jon Kohler wrote:
> >>> Add helper for calling skb_{put|reserve} to reduce repetitive pattern
> >>> across various drivers.
> >>>
> >>> Plumb into tap and tun to start.
> >>>
> >>> No functional change intended.
> >>>
> >>> Signed-off-by: Jon Kohler <jon@nutanix.com>
> >>> ---
> >>> drivers/net/tap.c | 3 +--
> >>> drivers/net/tun.c | 3 +--
> >>> include/net/xdp.h | 8 ++++++++
> >>> net/core/xdp.c | 3 +--
> >>> 4 files changed, 11 insertions(+), 6 deletions(-)
> >> Subjective, but I prefer the existing code. I understand what
> >> skb_reserve and skb_put do. While xdp_skb_reserve_put adds a layer of
> >> indirection that I'd have to follow.
> >> Sometimes deduplication makes sense, sometimes the indirection adds
> >> more mental load than it's worth. In this case the code savings are
> >> small. As said, subjective. Happy to hear other opinions.
> >
> > +1, agree with Willem
>
> That’s a fair point. I was also toying with the idea of something like
> this instead:
>
> e.g.
> xdp_headroom(xdp) == xdp->data - xdp->data_hard_start
> … similar to skb_headroom
>
> xdp_length_base(xdp) == xdp->data_end - xdp->data
> … similar to xdp_get_buff_len, but doesn’t look at frags
>
> then we could do:
> skb_reserve(skb, xdp_headroom(xdp));
> skb_put(skb, xdp_length_base(xdp));
>
> Names TBD of course, but thoughts?
>
> That way we keep skb_reserve/put just the same, but have
> a nice helper like we do for skb_headroom() already
I like the idea of xdp_headroom and xdk_headlen, similar to
skb_headroom and skb_headlen.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] xdp: add xdp_skb_reserve_put helper
2025-04-30 19:04 ` Willem de Bruijn
@ 2025-04-30 19:09 ` Jon Kohler
0 siblings, 0 replies; 6+ messages in thread
From: Jon Kohler @ 2025-04-30 19:09 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Daniel Borkmann, Jason Wang, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexei Starovoitov,
Jesper Dangaard Brouer, John Fastabend, Simon Horman,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
bpf@vger.kernel.org
> On Apr 30, 2025, at 3:04 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Jon Kohler wrote:
>>
>>
>>> On Apr 30, 2025, at 2:40 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>
>>> !-------------------------------------------------------------------|
>>> CAUTION: External Email
>>>
>>> |-------------------------------------------------------------------!
>>>
>>> On 4/30/25 8:25 PM, Willem de Bruijn wrote:
>>>> Jon Kohler wrote:
>>>>> Add helper for calling skb_{put|reserve} to reduce repetitive pattern
>>>>> across various drivers.
>>>>>
>>>>> Plumb into tap and tun to start.
>>>>>
>>>>> No functional change intended.
>>>>>
>>>>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>>>>> ---
>>>>> drivers/net/tap.c | 3 +--
>>>>> drivers/net/tun.c | 3 +--
>>>>> include/net/xdp.h | 8 ++++++++
>>>>> net/core/xdp.c | 3 +--
>>>>> 4 files changed, 11 insertions(+), 6 deletions(-)
>>>> Subjective, but I prefer the existing code. I understand what
>>>> skb_reserve and skb_put do. While xdp_skb_reserve_put adds a layer of
>>>> indirection that I'd have to follow.
>>>> Sometimes deduplication makes sense, sometimes the indirection adds
>>>> more mental load than it's worth. In this case the code savings are
>>>> small. As said, subjective. Happy to hear other opinions.
>>>
>>> +1, agree with Willem
>>
>> That’s a fair point. I was also toying with the idea of something like
>> this instead:
>>
>> e.g.
>> xdp_headroom(xdp) == xdp->data - xdp->data_hard_start
>> … similar to skb_headroom
>>
>> xdp_length_base(xdp) == xdp->data_end - xdp->data
>> … similar to xdp_get_buff_len, but doesn’t look at frags
>>
>> then we could do:
>> skb_reserve(skb, xdp_headroom(xdp));
>> skb_put(skb, xdp_length_base(xdp));
>>
>> Names TBD of course, but thoughts?
>>
>> That way we keep skb_reserve/put just the same, but have
>> a nice helper like we do for skb_headroom() already
>
> I like the idea of xdp_headroom and xdk_headlen, similar to
> skb_headroom and skb_headlen.
>
Sold! I’ll cook it up
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-04-30 19:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-30 18:29 [PATCH net-next] xdp: add xdp_skb_reserve_put helper Jon Kohler
2025-04-30 18:25 ` Willem de Bruijn
2025-04-30 18:40 ` Daniel Borkmann
2025-04-30 18:45 ` Jon Kohler
2025-04-30 19:04 ` Willem de Bruijn
2025-04-30 19:09 ` Jon Kohler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox