Re: [PATCH 1/3 net-next] linux: Add skb_frag_t page_offset accessors

On 29 Jul 2019, at 13:50, Jakub Kicinski wrote:

On Mon, 29 Jul 2019 10:19:39 -0700, Jonathan Lemon wrote:

quoted

Add skb_frag_off(), skb_frag_off_add(), skb_frag_off_set(),
and skb_frag_off_set_from() accessors for page_offset.

Signed-off-by: Jonathan Lemon <redacted>
---
 include/linux/skbuff.h | 61 
++++++++++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 5 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 718742b1c505..7d94a78067ee 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -331,7 +331,7 @@ static inline void skb_frag_size_set(skb_frag_t 

*frag, unsigned int size)
 }

 /**
- * skb_frag_size_add - Incrementes the size of a skb fragment by 
%delta
+ * skb_frag_size_add - Increments the size of a skb fragment by 
%delta
  * @frag: skb fragment
  * @delta: value to add
  */

@@ -2857,6 +2857,46 @@ static inline void 

skb_propagate_pfmemalloc(struct page *page,
 		skb->pfmemalloc = true;
 }

+/**
+ * skb_frag_off - Returns the offset of a skb fragment
+ * @frag: the paged fragment
+ */
+static inline unsigned int skb_frag_off(const skb_frag_t *frag)
+{
+	return frag->page_offset;
+}
+
+/**
+ * skb_frag_off_add - Increments the offset of a skb fragment by 
%delta

I realize you're following the existing code, but should we perhaps 
use
the latest kdoc syntax? '()' after function name, and args should have
'@' prefix, '%' would be for constants.

That would be a task for a different cleanup.  Not that I disagree with 
you,
but there's also nothing worse than mixing styles in the same file.

quoted

+ * @frag: skb fragment
+ * @delta: value to add
+ */
+static inline void skb_frag_off_add(skb_frag_t *frag, int delta)
+{
+	frag->page_offset += delta;
+}
+
+/**
+ * skb_frag_off_set - Sets the offset of a skb fragment
+ * @frag: skb fragment
+ * @offset: offset of fragment
+ */
+static inline void skb_frag_off_set(skb_frag_t *frag, unsigned int 
offset)
+{
+	frag->page_offset = offset;
+}
+
+/**
+ * skb_frag_off_set_from - Sets the offset of a skb fragment from 
another fragment
+ * @fragto: skb fragment where offset is set
+ * @fragfrom: skb fragment offset is copied from
+ */
+static inline void skb_frag_off_set_from(skb_frag_t *fragto,
+					 const skb_frag_t *fragfrom)

skb_frag_off_copy() ?

That was my initial inclination, but due to the often overloaded
connotations of the word "copy", opted to use the same "set" verbiage
that existed in the other functions.

quoted

+{
+	fragto->page_offset = fragfrom->page_offset;
+}
+
 /**
  * skb_frag_page - retrieve the page referred to by a paged fragment
  * @frag: the paged fragment

@@ -2923,7 +2963,7 @@ static inline void skb_frag_unref(struct 

sk_buff *skb, int f)
  */
 static inline void *skb_frag_address(const skb_frag_t *frag)
 {
-	return page_address(skb_frag_page(frag)) + frag->page_offset;
+	return page_address(skb_frag_page(frag)) + skb_frag_off(frag);
 }

 /**

@@ -2939,7 +2979,18 @@ static inline void 

*skb_frag_address_safe(const skb_frag_t *frag)
 	if (unlikely(!ptr))
 		return NULL;

-	return ptr + frag->page_offset;
+	return ptr + skb_frag_off(frag);
+}
+
+/**
+ * skb_frag_page_set_from - sets the page in a fragment from another 
fragment

skb_frag_page_copy() ?

Same reasoning as above.

quoted

+ * @fragto: skb fragment where page is set
+ * @fragfrom: skb fragment page is copied from
+ */
+static inline void skb_frag_page_set_from(skb_frag_t *fragto,
+					  const skb_frag_t *fragfrom)
+{
+	fragto->bv_page = fragfrom->bv_page;
 }

 /**