On Sun, May 25, 2025 at 10:44:43AM -0700, Mina Almasry wrote:
> On Sun, May 25, 2025 at 6:04 AM Dragos Tatulea <dtatulea@xxxxxxxxxx> wrote:
> >
> > 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@xxxxxxxxxx> wrote:
> > > >
> > > > From: Dragos Tatulea <dtatulea@xxxxxxxxxx>
> > > >
> > > > Allow drivers that have moved over to netmem to do fragment coalescing.
> > > >
> > > > Signed-off-by: Dragos Tatulea <dtatulea@xxxxxxxxxx>
> > > > Signed-off-by: Cosmin Ratiu <cratiu@xxxxxxxxxx>
> > > > Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
> > > > ---
> > > > 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?
> >
>
> [net|dev]mems can now be in the TX path too:
> https://lore.kernel.org/netdev/20250508004830.4100853-1-almasrymina@xxxxxxxxxx/
>
> And even without explicit TX support, IIUC from Kuba RX packets can
> always be looped back to the TX path via forwarding or tc and what
> not. So let's leave the skb_zcopy check in the common path for now
> unless we're sure the move is safe.
>
Makes sense. Will be done.
Thanks,
Dragos
