netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "Ricardo Martinez" <ricardo.martinez@linux.intel.com>,
	netdev@vger.kernel.org, linux-wireless@vger.kernel.org,
	"David Miller" <davem@davemloft.net>,
	"Johannes Berg" <johannes@sipsolutions.net>,
	"Loic Poulain" <loic.poulain@linaro.org>,
	"M Chetan Kumar" <m.chetan.kumar@intel.com>,
	"Devegowda, Chandrashekar" <chandrashekar.devegowda@intel.com>,
	"Intel Corporation" <linuxwwan@intel.com>,
	chiranjeevi.rapolu@linux.intel.com,
	"Haijun Liu ( 刘海军)" <haijun.liu@mediatek.com>,
	"Hanania, Amir" <amir.hanania@intel.com>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
	"Sharma, Dinesh" <dinesh.sharma@intel.com>,
	"Lee, Eliot" <eliot.lee@intel.com>,
	"Jarvinen, Ilpo Johannes" <ilpo.johannes.jarvinen@intel.com>,
	"Veleta, Moises" <moises.veleta@intel.com>,
	"Bossart, Pierre-louis" <pierre-louis.bossart@intel.com>,
	"Sethuraman, Muralidharan" <muralidharan.sethuraman@intel.com>,
	"Mishra, Soumya Prakash" <Soumya.Prakash.Mishra@intel.com>,
	"Kancharla, Sreehari" <sreehari.kancharla@intel.com>,
	"Sahu, Madhusmita" <madhusmita.sahu@intel.com>
Subject: Re: [PATCH net-next v8 02/14] net: skb: introduce skb_data_area_size()
Date: Tue, 10 May 2022 16:37:27 +0300	[thread overview]
Message-ID: <CAHNKnsTmH-rGgWi3jtyC=ktM1DW2W1VJkYoTMJV2Z_Bt498bsg@mail.gmail.com> (raw)
In-Reply-To: <20220509125008.6d1c3b9b@kernel.org>

On Mon, May 9, 2022 at 10:50 PM Jakub Kicinski <kuba@kernel.org> 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

  parent reply	other threads:[~2022-05-10 13:50 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06 18:12 [PATCH net-next v8 00/14] net: wwan: t7xx: PCIe driver for MediaTek M.2 modem Ricardo Martinez
2022-05-06 18:12 ` [PATCH net-next v8 01/14] list: Add list_next_entry_circular() and list_prev_entry_circular() Ricardo Martinez
2022-05-06 18:12 ` [PATCH net-next v8 02/14] net: skb: introduce skb_data_area_size() Ricardo Martinez
2022-05-09 16:49   ` Jakub Kicinski
2022-05-09 18:34     ` Sergey Ryazanov
2022-05-09 19:50       ` Jakub Kicinski
2022-05-09 21:37         ` Martinez, Ricardo
2022-05-10 10:01           ` Andy Shevchenko
2022-05-10 13:37         ` Sergey Ryazanov [this message]
2022-05-06 18:12 ` [PATCH net-next v8 03/14] net: wwan: t7xx: Add control DMA interface Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 04/14] net: wwan: t7xx: Add core components Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 05/14] net: wwan: t7xx: Add port proxy infrastructure Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 06/14] net: wwan: t7xx: Add control port Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 07/14] net: wwan: t7xx: Add AT and MBIM WWAN ports Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 08/14] net: wwan: t7xx: Data path HW layer Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 09/14] net: wwan: t7xx: Add data path interface Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 10/14] net: wwan: t7xx: Add WWAN network interface Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 11/14] net: wwan: t7xx: Introduce power management Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 12/14] net: wwan: t7xx: Runtime PM Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 13/14] net: wwan: t7xx: Device deep sleep lock/unlock Ricardo Martinez
2022-05-06 18:13 ` [PATCH net-next v8 14/14] net: wwan: t7xx: Add maintainers and documentation Ricardo Martinez
2022-05-09 11:00 ` [PATCH net-next v8 00/14] net: wwan: t7xx: PCIe driver for MediaTek M.2 modem patchwork-bot+netdevbpf

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

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

  git send-email \
    --in-reply-to='CAHNKnsTmH-rGgWi3jtyC=ktM1DW2W1VJkYoTMJV2Z_Bt498bsg@mail.gmail.com' \
    --to=ryazanov.s.a@gmail.com \
    --cc=Soumya.Prakash.Mishra@intel.com \
    --cc=amir.hanania@intel.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=chandrashekar.devegowda@intel.com \
    --cc=chiranjeevi.rapolu@linux.intel.com \
    --cc=davem@davemloft.net \
    --cc=dinesh.sharma@intel.com \
    --cc=eliot.lee@intel.com \
    --cc=haijun.liu@mediatek.com \
    --cc=ilpo.johannes.jarvinen@intel.com \
    --cc=johannes@sipsolutions.net \
    --cc=kuba@kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linuxwwan@intel.com \
    --cc=loic.poulain@linaro.org \
    --cc=m.chetan.kumar@intel.com \
    --cc=madhusmita.sahu@intel.com \
    --cc=moises.veleta@intel.com \
    --cc=muralidharan.sethuraman@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=pierre-louis.bossart@intel.com \
    --cc=ricardo.martinez@linux.intel.com \
    --cc=sreehari.kancharla@intel.com \
    --subject='Re: [PATCH net-next v8 02/14] net: skb: introduce skb_data_area_size()' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).