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)
 {
 	int err;
 
-	err = skb_do_copy_data_nocache(sk, skb, from, page_address(page) + off,
-				       copy, skb->len);
+	err = skb_do_copy_data_nocache(sk, skb, from, va, copy, skb->len);
 	if (err)
 		return err;
 
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4f77bd862e95..8b03d8f48ac9 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1218,10 +1218,9 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 			if (!copy)
 				goto wait_for_space;
 
-			err = skb_copy_to_page_nocache(sk, &msg->msg_iter, skb,
-						       pfrag->page,
-						       pfrag->offset,
-						       copy);
+			err = skb_copy_to_va_nocache(sk, &msg->msg_iter, skb,
+						     page_address(pfrag->page) +
+						     pfrag->offset, copy);
 			if (err)
 				goto do_error;
 
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index d4118c796290..f4462cf88ed5 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -856,10 +856,9 @@ static int kcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 			if (!sk_wmem_schedule(sk, copy))
 				goto wait_for_memory;
 
-			err = skb_copy_to_page_nocache(sk, &msg->msg_iter, skb,
-						       pfrag->page,
-						       pfrag->offset,
-						       copy);
+			err = skb_copy_to_va_nocache(sk, &msg->msg_iter, skb,
+						     page_address(pfrag->page) +
+						     pfrag->offset, copy);
 			if (err)
 				goto out_error;
 
-- 
2.34.1

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
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 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.

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?

On Sat, Oct 5, 2024 at 6:44 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote:
>
> 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

Maybe "sk_copy_to_skb_frag_nocache" or something along those lines
would be an even better naming for it. Basically I just want to avoid
having the two very different types of functions sound like they might
be related.

As it stands it and the other 2 functions related to it are an outlier
in the header file as most everything else in the header file starts
with sk_ anyway as it isn't skbuff.h so it doesn't make sense to have
skb_ functions living in it.

> > 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?

It isn't. If we were messing with linear data we shouldn't be updating
data_len. This is the kind of thing that worries me about this as it
can easily lead to misuse.

The two functions are different in several important ways.
Specifically one is meant to copy to the headers, and the other is
meant to copy to detached frags. In addition the
skb_copy_to_linear_data doesn't do the skb->len manipulation nor the
socket manipulation.

> >
> > 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?

I could probably live with sk_copy_to_skb_data_nocache since we also
refer to the section after the page section with data_len. The basic
idea is we are wanting to define what the function does with the
function name rather than just report the arguments it is accepting.

On 10/7/2024 12:18 AM, Alexander Duyck wrote:

...

> 
> I could probably live with sk_copy_to_skb_data_nocache since we also
> refer to the section after the page section with data_len. The basic
> idea is we are wanting to define what the function does with the
> function name rather than just report the arguments it is accepting.

Yes, looking more closely:
skb_add_data_nocache() does memcpy'ing to skb->data and update skb->len
only by calling skb_put(), and skb_copy_to_page_nocache() does
memcpy'ing to skb frag by updating both skb->len and skb->data_len
through the calling of skb_len_add().

Perhaps skb_add_frag_nocache() might seems a better name for now, and
the 'sk_' prefix might be done in the future if it does make sense.

On Mon, Oct 7, 2024 at 7:29 AM Yunsheng Lin <yunshenglin0825@gmail.com> wrote:
>
> On 10/7/2024 12:18 AM, Alexander Duyck wrote:
>
> ...
>
> >
> > I could probably live with sk_copy_to_skb_data_nocache since we also
> > refer to the section after the page section with data_len. The basic
> > idea is we are wanting to define what the function does with the
> > function name rather than just report the arguments it is accepting.
>
> Yes, looking more closely:
> skb_add_data_nocache() does memcpy'ing to skb->data and update skb->len
> only by calling skb_put(), and skb_copy_to_page_nocache() does
> memcpy'ing to skb frag by updating both skb->len and skb->data_len
> through the calling of skb_len_add().
>
> Perhaps skb_add_frag_nocache() might seems a better name for now, and
> the 'sk_' prefix might be done in the future if it does make sense.

That works for me.