Re: [RFC 6/9] staging: dpaa2-switch: add .ndo_start_xmit() callback

From: Ioana Ciornei
Date: Thu Nov 05 2020 - 03:26:03 EST


On Thu, Nov 05, 2020 at 02:04:39AM +0100, Andrew Lunn wrote:
> > +static int dpaa2_switch_build_single_fd(struct ethsw_core *ethsw,
> > + struct sk_buff *skb,
> > + struct dpaa2_fd *fd)
> > +{
> > + struct device *dev = ethsw->dev;
> > + struct sk_buff **skbh;
> > + dma_addr_t addr;
> > + u8 *buff_start;
> > + void *hwa;
> > +
> > + buff_start = PTR_ALIGN(skb->data - DPAA2_SWITCH_TX_DATA_OFFSET -
> > + DPAA2_SWITCH_TX_BUF_ALIGN,
> > + DPAA2_SWITCH_TX_BUF_ALIGN);
> > +
> > + /* Clear FAS to have consistent values for TX confirmation. It is
> > + * located in the first 8 bytes of the buffer's hardware annotation
> > + * area
> > + */
> > + hwa = buff_start + DPAA2_SWITCH_SWA_SIZE;
> > + memset(hwa, 0, 8);
> > +
> > + /* Store a backpointer to the skb at the beginning of the buffer
> > + * (in the private data area) such that we can release it
> > + * on Tx confirm
> > + */
> > + skbh = (struct sk_buff **)buff_start;
> > + *skbh = skb;
>
> Where is the TX confirm which uses this stored pointer. I don't see it
> in this file.
>

The Tx confirm - dpaa2_switch_tx_conf() - is added in patch 5/9.

> It can be expensive to store pointer like this in buffers used for
> DMA.

Yes, it is. But the hardware does not give us any other indication that
a packet was actually sent so that we can move ahead with consuming the
initial skb.

> It has to be flushed out of the cache here as part of the
> send. Then the TX complete needs to invalidate and then read it back
> into the cache. Or you use coherent memory which is just slow.
>
> It can be cheaper to keep a parallel ring in cacheable memory which
> never gets flushed.

I'm afraid I don't really understand your suggestion. In this parallel
ring I would keep the skb pointers of all frames which are in-flight?
Then, when a packet is received on the Tx confirmation queue I would
have to loop over the parallel ring and determine somehow which skb was
this packet initially associated to. Isn't this even more expensive?

Ioana