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@gmail.com>
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 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)
 {
 	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 82cc4a5633ce..bc36f02cac0d 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1219,10 +1219,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_add_frag_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..e74ad63f0189 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_add_frag_nocache(sk, &msg->msg_iter, skb,
+						   page_address(pfrag->page) +
+						   pfrag->offset, copy);
 			if (err)
 				goto out_error;
 
-- 
2.33.0

On Tue, Oct 8, 2024 at 4:27 AM Yunsheng Lin <linyunsheng@huawei.com> 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@gmail.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
>  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.

On 2024/10/10 7:40, Alexander Duyck wrote:
> On Tue, Oct 8, 2024 at 4:27 AM Yunsheng Lin <linyunsheng@huawei.com> 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@gmail.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> ---
>>  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@mail.gmail.com/

On Thu, Oct 10, 2024 at 4:32 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2024/10/10 7:40, Alexander Duyck wrote:
> > On Tue, Oct 8, 2024 at 4:27 AM Yunsheng Lin <linyunsheng@huawei.com> 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@gmail.com>
> >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> >> ---
> >>  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@mail.gmail.com/

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.