Re: [PATCH net-next V2 02/11] net: Add skb_can_coalesce for netmem

Dragos Tatulea
2025-05-25T13:04:11Z

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@nvidia.com> wrote:
> >
> > From: Dragos Tatulea <dtatulea@nvidia.com>
> >
> > Allow drivers that have moved over to netmem to do fragment coalescing.
> >
> > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
> > Signed-off-by: Tariq Toukan <tariqt@nvidia.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