netdev - Re: [PATCH net-next V2 02/11] net: Add skb_can

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c677zoajklqi3dg7wtnyw65licssvxxt3lmz5hvzfw3sm6w32g@pfd2ynqjkyov>
Date: Sun, 25 May 2025 13:03:53 +0000
From: Dragos Tatulea <dtatulea@...dia.com>
To: Mina Almasry <almasrymina@...gle.com>, 
	Tariq Toukan <tariqt@...dia.com>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Eric Dumazet <edumazet@...gle.com>, Andrew Lunn <andrew+netdev@...n.ch>, 
	Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>, 
	Richard Cochran <richardcochran@...il.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard Brouer <hawk@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, netdev@...r.kernel.org, linux-rdma@...r.kernel.org, 
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org, Moshe Shemesh <moshe@...dia.com>, 
	Mark Bloch <mbloch@...dia.com>, Gal Pressman <gal@...dia.com>, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net-next V2 02/11] net: Add skb_can_coalesce for netmem

On Thu, May 22, 2025 at 04:09:35PM -0700, Mina Almasry wrote:
> On Thu, May 22, 2025 at 2:43 PM Tariq Toukan <tariqt@...dia.com> wrote:
> >
> > From: Dragos Tatulea <dtatulea@...dia.com>
> >
> > Allow drivers that have moved over to netmem to do fragment coalescing.
> >
> > Signed-off-by: Dragos Tatulea <dtatulea@...dia.com>
> > Signed-off-by: Cosmin Ratiu <cratiu@...dia.com>
> > Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> > ---
> >  include/linux/skbuff.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 5520524c93bf..e8e2860183b4 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3887,6 +3887,18 @@ static inline bool skb_can_coalesce(struct sk_buff *skb, int i,
> >         return false;
> >  }
> >
> > +static inline bool skb_can_coalesce_netmem(struct sk_buff *skb, int i,
> > +                                          const netmem_ref netmem, int off)
> > +{
> > +       if (i) {
> > +               const skb_frag_t *frag = &skb_shinfo(skb)->frags[i - 1];
> > +
> > +               return netmem == skb_frag_netmem(frag) &&
> > +                      off == skb_frag_off(frag) + skb_frag_size(frag);
> > +       }
> > +       return false;
> > +}
> > +
> 
> Can we limit the code duplication by changing skb_can_coalesce to call
> skb_can_coalesce_netmem? Or is that too bad for perf?
>
> static inline bool skb_can_coalesce(struct sk_buff *skb, int i, const
> struct page *page, int off) {
>     skb_can_coalesce_netmem(skb, i, page_to_netmem(page), off);
> }
> 
> It's always safe to cast a page to netmem.
>
I think it makes sense and I don't see an issue with perf as everything
stays inline and the cast should be free.

As netmems are used only for rx and skb_zcopy() seems to be used
only for tx (IIUC), maybe it makes sense to keep the skb_zcopy() check
within skb_can_coalesce(). Like below. Any thoughts?

--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3873,15 +3873,22 @@ static inline bool skb_can_coalesce(struct sk_buff *skb, int i,
 {
 	if (skb_zcopy(skb))
 		return false;
-	if (i) {
-		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i - 1];
 
-		return page == skb_frag_page(frag) &&
-		       off == skb_frag_off(frag) + skb_frag_size(frag);
-	}
-	return false;
+	return skb_can_coalesce_netmem(skb, i, page_to_netmem(page), off);
 }
 
+   static inline bool skb_can_coalesce_netmem(struct sk_buff *skb, int i,
+                                              const netmem_ref netmem, int off)
+   {
+          if (i) {
+                  const skb_frag_t *frag = &skb_shinfo(skb)->frags[i - 1];
+
+                  return netmem == skb_frag_netmem(frag) &&
+                         off == skb_frag_off(frag) + skb_frag_size(frag);
+          }
+          return false;
+   }
+

Thanks,
Dragos