Re: [PATCH net-next v19 09/14] net: rename skb_copy_to_page_nocache() helper

[Yunsheng Lin <yunshenglin0825@gmail.com>]

On 10/4/2024 11:00 AM, Alexander Duyck wrote:
> On Tue, Oct 1, 2024 at 12:59 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote:
>>
>> Rename skb_copy_to_page_nocache() to skb_copy_to_va_nocache()
>> to avoid calling virt_to_page() as we are about to pass virtual
>> address directly.
>>
>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> ---
>>   include/net/sock.h | 10 ++++------
>>   net/ipv4/tcp.c     |  7 +++----
>>   net/kcm/kcmsock.c  |  7 +++----
>>   3 files changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index c58ca8dd561b..7d0b606d6251 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -2185,15 +2185,13 @@ static inline int skb_add_data_nocache(struct sock *sk, struct sk_buff *skb,
>>          return err;
>>   }
>>
>> -static inline int skb_copy_to_page_nocache(struct sock *sk, struct iov_iter *from,
>> -                                          struct sk_buff *skb,
>> -                                          struct page *page,
>> -                                          int off, int copy)
>> +static inline int skb_copy_to_va_nocache(struct sock *sk, struct iov_iter *from,
>> +                                        struct sk_buff *skb, char *va,
>> +                                        int copy)
>>   {
> 
> This new naming is kind of confusing. Currently the only other
> "skb_copy_to" functions are skb_copy_to_linear_data and
> skb_copy_to_linear_data_offset. The naming before basically indicated

I am not sure if the above "skb_copy_to" functions are really related
here, as they are in include/linux/skbuff.h and don't take '*sk' as
first input param.

As "skb_copy_to" function in include/net/sock.h does take '*sk' as first
input param, perhaps the "skb_copy_to" functions in include/net/sock.h
can be renamed to "sk_skb_copy_to" in the future as most of functions
do in include/net/sock.h

> which part of the skb the data was being copied into. So before we
> were copying into the "page" frags. With the new naming this function
> is much less clear as technically the linear data can also be a
> virtual address.

I guess it is ok to use it for linear data if there is a need, why
invent another function for the linear data when both linear data and
non-linear data can be used as a virtual address?

> 
> I would recommend maybe replacing "va" with "frag", "page_frag" or
> maybe "pfrag" as what we are doing is copying the data to one of the
> pages in the paged frags section of the skb before they are added to
> the skb itself.

Don't "frag", "page_frag" or "pfrag" also seem confusing enough that
it does not take any 'frag' as the input param?

Does skb_copy_data() make more sense here as it can work on both
linear and non-linear data, as skb_do_copy_data_nocache() and
skb_copy_to_page_nocache() in the same header file seem to have a
similar style?