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

[Alexander Duyck]

On Thu, Oct 10, 2024 at 4:32 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> 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/

Sorry, I overlooked the part where you mentioned skb_add_frag_nocache.
For some reason I was thinking you were going with the
skb_copy_to_data_nocache. The issue is that adding a frag has a
meaning and sounds similar to other existing functions. By sticking
with the data_nocache suffix it stays closer to the other similar
functions.