Helper to calculate the linear data space in the skb.
Signed-off-by: Ricardo Martinez <[email protected]>
Reviewed-by: Sergey Ryazanov <[email protected]>
---
include/linux/skbuff.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 5c2599e3fe7d..d58669d6cb91 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1665,6 +1665,11 @@ static inline void skb_set_end_offset(struct sk_buff *skb, unsigned int offset)
}
#endif
+static inline unsigned int skb_data_area_size(struct sk_buff *skb)
+{
+ return skb_end_pointer(skb) - skb->data;
+}
+
struct ubuf_info *msg_zerocopy_realloc(struct sock *sk, size_t size,
struct ubuf_info *uarg);
--
2.25.1
On Fri, 6 May 2022 11:12:58 -0700 Ricardo Martinez wrote:
> Helper to calculate the linear data space in the skb.
>
> Signed-off-by: Ricardo Martinez <[email protected]>
> Reviewed-by: Sergey Ryazanov <[email protected]>
> ---
> include/linux/skbuff.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 5c2599e3fe7d..d58669d6cb91 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1665,6 +1665,11 @@ static inline void skb_set_end_offset(struct sk_buff *skb, unsigned int offset)
> }
> #endif
>
> +static inline unsigned int skb_data_area_size(struct sk_buff *skb)
> +{
> + return skb_end_pointer(skb) - skb->data;
> +}
Not a great name, skb->data_len is the length of paged data.
There is no such thing as "data area", data is just a pointer
somewhere into skb->head.
Why do you need this? Why can't you use the size you passed
to the dev_alloc_skb() like everyone else?
Hello Jakub,
On Mon, May 9, 2022 at 7:49 PM Jakub Kicinski <[email protected]> wrote:
> On Fri, 6 May 2022 11:12:58 -0700 Ricardo Martinez wrote:
>> Helper to calculate the linear data space in the skb.
>>
>> Signed-off-by: Ricardo Martinez <[email protected]>
>> Reviewed-by: Sergey Ryazanov <[email protected]>
>> ---
>> include/linux/skbuff.h | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 5c2599e3fe7d..d58669d6cb91 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -1665,6 +1665,11 @@ static inline void skb_set_end_offset(struct sk_buff *skb, unsigned int offset)
>> }
>> #endif
>>
>> +static inline unsigned int skb_data_area_size(struct sk_buff *skb)
>> +{
>> + return skb_end_pointer(skb) - skb->data;
>> +}
>
> Not a great name, skb->data_len is the length of paged data.
> There is no such thing as "data area", data is just a pointer
> somewhere into skb->head.
What name would you recommend for this helper?
> Why do you need this? Why can't you use the size you passed
> to the dev_alloc_skb() like everyone else?
It was I who suggested to Ricardo to make this helper a common
function [1]. So let me begin the discussion, perhaps Ricardo will add
to my thoughts as the driver author.
There are many other places where authors do the same but without a
helper function:
$ grep -rn 'skb_end_pointer(.*) [-]' drivers/net/ | sort
drivers/net/ethernet/marvell/mv643xx_eth.c:628: size =
skb_end_pointer(skb) - skb->data;
drivers/net/ethernet/marvell/pxa168_eth.c:322: size =
skb_end_pointer(skb) - skb->data;
drivers/net/ethernet/micrel/ksz884x.c:4764: if (skb_end_pointer(skb) -
skb->data >= 50) {
drivers/net/ethernet/netronome/nfp/ccm_mbox.c:492: undersize =
max_reply_size - (skb_end_pointer(skb) - skb->data);
drivers/net/ethernet/nvidia/forcedeth.c:2073:
(skb_end_pointer(np->rx_skb[i].skb) -
drivers/net/ethernet/nvidia/forcedeth.c:5244: (skb_end_pointer(tx_skb)
- tx_skb->data),
drivers/net/veth.c:775: frame_sz = skb_end_pointer(skb) - skb->head;
There are at least two reasons to evaluate the linear data size from skb:
1) it is difficult to access the same variable that contains a size
during skb allocation (consider skb in a queue);
2) you may not have access to an allocation size because a driver does
not allocate that skb (consider a xmit path).
Eventually you found themself in a position where you need to carry
additional info along with skb. But, on the other hand, you can simply
calculate this value from the skb pointers themselves.
1. https://lore.kernel.org/netdev/CAHNKnsTr3aq1sgHnZQFL7-0uHMp3Wt4PMhVgTMSAiiXT=8p35A@mail.gmail.com/
--
Sergey
On Mon, 9 May 2022 21:34:58 +0300 Sergey Ryazanov wrote:
> >> +static inline unsigned int skb_data_area_size(struct sk_buff *skb)
> >> +{
> >> + return skb_end_pointer(skb) - skb->data;
> >> +}
> >
> > Not a great name, skb->data_len is the length of paged data.
> > There is no such thing as "data area", data is just a pointer
> > somewhere into skb->head.
>
> What name would you recommend for this helper?
We are assuming that skb->data is where we want to start to write
to so we own the skb. If it's a fresh skb skb->data == skb->tail.
If it's not fresh but recycled - skb->data is presumably reset
correctly, and then skb_reset_tail_pointer() has to be called. Either
way we give HW empty skbs, tailroom is an existing concept we can use.
> > Why do you need this? Why can't you use the size you passed
> > to the dev_alloc_skb() like everyone else?
>
> It was I who suggested to Ricardo to make this helper a common
> function [1]. So let me begin the discussion, perhaps Ricardo will add
> to my thoughts as the driver author.
>
> There are many other places where authors do the same but without a
> helper function:
>
> [...]
>
> There are at least two reasons to evaluate the linear data size from skb:
> 1) it is difficult to access the same variable that contains a size
> during skb allocation (consider skb in a queue);
Usually all the Rx skbs on the queue are equally sized so no need to
save the length per buffer, but perhaps that's not universally true.
> 2) you may not have access to an allocation size because a driver does
> not allocate that skb (consider a xmit path).
On Tx you should only map the data that's actually populated, for that
we have skb_headlen().
> Eventually you found themself in a position where you need to carry
> additional info along with skb. But, on the other hand, you can simply
> calculate this value from the skb pointers themselves.
>
> 1. https://lore.kernel.org/netdev/CAHNKnsTr3aq1sgHnZQFL7-0uHMp3Wt4PMhVgTMSAiiXT=8p35A@mail.gmail.com/
Fair enough, I didn't know more drivers are doing this.
We have two cases:
- for Tx - drivers should use skb_headlen();
- for Rx - I presume we are either dealing with fresh or correctly
reset skbs, so we can use skb_tailroom().
On 5/9/2022 12:50 PM, Jakub Kicinski wrote:
> On Mon, 9 May 2022 21:34:58 +0300 Sergey Ryazanov wrote:
>>>> +static inline unsigned int skb_data_area_size(struct sk_buff *skb)
>>>> +{
>>>> + return skb_end_pointer(skb) - skb->data;
>>>> +}
>>> Not a great name, skb->data_len is the length of paged data.
>>> There is no such thing as "data area", data is just a pointer
>>> somewhere into skb->head.
>> What name would you recommend for this helper?
> We are assuming that skb->data is where we want to start to write
> to so we own the skb. If it's a fresh skb skb->data == skb->tail.
> If it's not fresh but recycled - skb->data is presumably reset
> correctly, and then skb_reset_tail_pointer() has to be called. Either
> way we give HW empty skbs, tailroom is an existing concept we can use.
>
>>> Why do you need this? Why can't you use the size you passed
>>> to the dev_alloc_skb() like everyone else?
>> It was I who suggested to Ricardo to make this helper a common
>> function [1]. So let me begin the discussion, perhaps Ricardo will add
>> to my thoughts as the driver author.
>>
>> There are many other places where authors do the same but without a
>> helper function:
>>
>> [...]
>>
>> There are at least two reasons to evaluate the linear data size from skb:
>> 1) it is difficult to access the same variable that contains a size
>> during skb allocation (consider skb in a queue);
> Usually all the Rx skbs on the queue are equally sized so no need to
> save the length per buffer, but perhaps that's not universally true.
>
>> 2) you may not have access to an allocation size because a driver does
>> not allocate that skb (consider a xmit path).
> On Tx you should only map the data that's actually populated, for that
> we have skb_headlen().
>
>> Eventually you found themself in a position where you need to carry
>> additional info along with skb. But, on the other hand, you can simply
>> calculate this value from the skb pointers themselves.
>>
>> 1. https://lore.kernel.org/netdev/CAHNKnsTr3aq1sgHnZQFL7-0uHMp3Wt4PMhVgTMSAiiXT=8p35A@mail.gmail.com/
> Fair enough, I didn't know more drivers are doing this.
>
> We have two cases:
> - for Tx - drivers should use skb_headlen();
> - for Rx - I presume we are either dealing with fresh or correctly
> reset skbs, so we can use skb_tailroom().
Thanks for the information, looks like indeed we can avoid the helper in
t7xx driver by following those guidelines.
On Mon, May 09, 2022 at 02:37:26PM -0700, Martinez, Ricardo wrote:
> On 5/9/2022 12:50 PM, Jakub Kicinski wrote:
> > On Mon, 9 May 2022 21:34:58 +0300 Sergey Ryazanov wrote:
...
> > We have two cases:
> > - for Tx - drivers should use skb_headlen();
> > - for Rx - I presume we are either dealing with fresh or correctly
> > reset skbs, so we can use skb_tailroom().
> Thanks for the information, looks like indeed we can avoid the helper in
> t7xx driver by following those guidelines.
I think this can be done as a follow up patch.
--
With Best Regards,
Andy Shevchenko
On Mon, May 9, 2022 at 10:50 PM Jakub Kicinski <[email protected]> wrote:
> On Mon, 9 May 2022 21:34:58 +0300 Sergey Ryazanov wrote:
>>>> +static inline unsigned int skb_data_area_size(struct sk_buff *skb)
>>>> +{
>>>> + return skb_end_pointer(skb) - skb->data;
>>>> +}
>>>
>>> Not a great name, skb->data_len is the length of paged data.
>>> There is no such thing as "data area", data is just a pointer
>>> somewhere into skb->head.
>>
> > What name would you recommend for this helper?
>
> We are assuming that skb->data is where we want to start to write
> to so we own the skb. If it's a fresh skb skb->data == skb->tail.
> If it's not fresh but recycled - skb->data is presumably reset
> correctly, and then skb_reset_tail_pointer() has to be called. Either
> way we give HW empty skbs, tailroom is an existing concept we can use.
>
>>> Why do you need this? Why can't you use the size you passed
>>> to the dev_alloc_skb() like everyone else?
>>
>> It was I who suggested to Ricardo to make this helper a common
>> function [1]. So let me begin the discussion, perhaps Ricardo will add
>> to my thoughts as the driver author.
>>
>> There are many other places where authors do the same but without a
>> helper function:
>>
>> [...]
>>
>> There are at least two reasons to evaluate the linear data size from skb:
>> 1) it is difficult to access the same variable that contains a size
>> during skb allocation (consider skb in a queue);
>
> Usually all the Rx skbs on the queue are equally sized so no need to
> save the length per buffer, but perhaps that's not universally true.
>
>> 2) you may not have access to an allocation size because a driver does
>> not allocate that skb (consider a xmit path).
>
> On Tx you should only map the data that's actually populated, for that
> we have skb_headlen().
>
>> Eventually you found themself in a position where you need to carry
>> additional info along with skb. But, on the other hand, you can simply
>> calculate this value from the skb pointers themselves.
>>
>> 1. https://lore.kernel.org/netdev/CAHNKnsTr3aq1sgHnZQFL7-0uHMp3Wt4PMhVgTMSAiiXT=8p35A@mail.gmail.com/
>
> Fair enough, I didn't know more drivers are doing this.
>
> We have two cases:
> - for Tx - drivers should use skb_headlen();
> - for Rx - I presume we are either dealing with fresh or correctly
> reset skbs, so we can use skb_tailroom().
Make sense! Especially your remark that on Tx we should only map
populated data. This short API usage guide even answers my wonder why
the kernel still does not have something like skb_data_area_size().
Thank you for this short and clear clarification.
--
Sergey