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

[Yunsheng Lin]

On 2024/10/10 7:40, Alexander Duyck wrote:
> On Tue, Oct 8, 2024 at 4:27 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>>
>> Rename skb_copy_to_page_nocache() to skb_add_frag_nocache()
>> to avoid calling virt_to_page() as we are about to pass virtual
>> address directly.
>>
>> CC: Alexander Duyck <alexander.duyck@xxxxxxxxx>
>> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
>> ---
>>  include/net/sock.h | 9 +++------
>>  net/ipv4/tcp.c     | 7 +++----
>>  net/kcm/kcmsock.c  | 7 +++----
>>  3 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index e282127092ab..e0b4e2daca5d 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -2192,15 +2192,12 @@ 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_add_frag_nocache(struct sock *sk, struct iov_iter *from,
>> +                                      struct sk_buff *skb, char *va, int copy)
> 
> This is not adding a frag. It is copying to a frag. This naming is a
> hard no as there are functions that actually add frags to the skb and
> this is not what this is doing. It sounds like it should be some
> variant on skb_add_rx_frag and it isn't.
> 
> Instead of "_add_" I would suggest you stick with "_copy_to_" as the
> action as the alternative would be confusing as it implies you are
> going to be adding this to frags yourself.

I though we had reached a agreement in [1]? I guessed the 'That works
for me' only refer to the 'sk_' prefix?

The argumemt is that "skb_add_data_nocache() does memcpy'ing to skb->data
and update skb->len only by calling skb_put()" without calling something as
pskb_expand_head() to add more tailroom, so skb_add_frag_nocache is mirroring
that.

Does it mean skb_add_data_nocache() may be renamed to skb_copy_to_data_nocache()
in the future?

1. https://lore.kernel.org/all/CAKgT0Ue=tX+hKWiXQaM-6ypZ8fGvcUagGKfVrNGtRHVuhMX80g@xxxxxxxxxxxxxx/