Re: [PATCH v6 net-next 5/5] net: ethernet: ti: cpsw: add XDP support

[Ivan Khoronzhuk]

On Thu, Jul 04, 2019 at 12:49:38PM +0300, Ilias Apalodimas wrote:
> On Thu, Jul 04, 2019 at 12:43:30PM +0300, Ivan Khoronzhuk wrote:
> > On Thu, Jul 04, 2019 at 12:39:02PM +0300, Ilias Apalodimas wrote:
> > >On Thu, Jul 04, 2019 at 11:19:39AM +0200, Jesper Dangaard Brouer wrote:
> > >>On Wed,  3 Jul 2019 13:19:03 +0300
> > >>Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxxxxxx> wrote:
> > >>
> > >>> Add XDP support based on rx page_pool allocator, one frame per page.
> > >>> Page pool allocator is used with assumption that only one rx_handler
> > >>> is running simultaneously. DMA map/unmap is reused from page pool
> > >>> despite there is no need to map whole page.
> > >>>
> > >>> Due to specific of cpsw, the same TX/RX handler can be used by 2
> > >>> network devices, so special fields in buffer are added to identify
> > >>> an interface the frame is destined to. Thus XDP works for both
> > >>> interfaces, that allows to test xdp redirect between two interfaces
> > >>> easily. Aslo, each rx queue have own page pools, but common for both
> > >>> netdevs.
> > >>>
> > >>> XDP prog is common for all channels till appropriate changes are added
> > >>> in XDP infrastructure. Also, once page_pool recycling becomes part of
> > >>> skb netstack some simplifications can be added, like removing
> > >>> page_pool_release_page() before skb receive.
> > >>>
> > >>> In order to keep rx_dev while redirect, that can be somehow used in
> > >>> future, do flush in rx_handler, that allows to keep rx dev the same
> > >>> while reidrect. It allows to conform with tracing rx_dev pointed
> > >>> by Jesper.
> > >>
> > >>So, you simply call xdp_do_flush_map() after each xdp_do_redirect().
> > >>It will kill RX-bulk and performance, but I guess it will work.
> > >>
> > >>I guess, we can optimized it later, by e.g. in function calling
> > >>cpsw_run_xdp() have a variable that detect if net_device changed
> > >>(priv->ndev) and then call xdp_do_flush_map() when needed.
> > >I tried something similar on the netsec driver on my initial development.
> > >On the 1gbit speed NICs i saw no difference between flushing per packet vs
> > >flushing on the end of the NAPI handler.
> > >The latter is obviously better but since the performance impact is negligible on
> > >this particular NIC, i don't think this should be a blocker.
> > >Please add a clear comment on this and why you do that on this driver,
> > >so people won't go ahead and copy/paste this approach
> > Sry, but I did this already, is it not enouph?
> The flush *must* happen there to avoid messing the following layers. The comment
> says something like 'just to be sure'. It's not something that might break, it's
> something that *will* break the code and i don't think that's clear with the
> current comment.
>
> So i'd prefer something like
> 'We must flush here, per packet, instead of doing it in bulk at the end of
> the napi handler.The RX devices on this particular hardware is sharing a
> common queue, so the incoming device might change per packet'
Sounds good, will replace on it.

--
Regards,
Ivan Khoronzhuk