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

[Yunsheng Lin]

On 10/4/2024 11:00 AM, Alexander Duyck wrote:
> On Tue, Oct 1, 2024 at 12:59 AM Yunsheng Lin <yunshenglin0825@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> > ---
> >   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?