All of lore.kernel.org
 help / color / mirror / Atom feed
From: brouer@redhat.com (Jesper Dangaard Brouer)
To: linux-arm-kernel@lists.infradead.org
Subject: [net PATCH] Revert "net: thunderx: Add support for xdp redirect"
Date: Wed, 14 Feb 2018 09:49:15 +0100	[thread overview]
Message-ID: <20180214094915.10fc6c36@redhat.com> (raw)
In-Reply-To: <CA+sq2CfYosrbZUEb0hLZS1u6tiz+m4oxEXF3-43ZxyoHq=cAVg@mail.gmail.com>

On Wed, 14 Feb 2018 01:05:16 +0530
Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:

> On Tue, Feb 13, 2018 at 10:29 PM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > This reverts commit aa136d0c82fcd6af14535853c30e219e02b2692d.
> >
> > As I previously[1] pointed out this implementation of XDP_REDIRECT is
> > wrong.  XDP_REDIRECT is a facility that must work between different
> > NIC drivers.  Another NIC driver can call ndo_xdp_xmit/nicvf_xdp_xmit,
> > but your driver patch assumes payload data (at top of page) will
> > contain a queue index and a DMA addr, this is not true and worse will
> > likely contain garbage.
> >
> > Given you have not fixed this in due time (just reached v4.16-rc1),
> > the only option I see is a revert.  
> 
> Sorry that i missed your email ie didn't respond.
>
> This driver is not for a generic PCI endpoint NIC, it's an on-silicon
> ethernet device found only on Cavium's ThunderX or OCTEONTX SOCs
> which supports upto 16 ports. XDP_REDIRECT here is mainly aimed at
> forwarding packets from one port to another
> port of same type.
> 
> The only way I could have avoided the payload data is by unmapping
> and remapping of DMA buffer which is quite expensive especially when
> IOMMU is enabled. So I thought this small optimization should be
> acceptable.

It is good that you bring up the need to avoid this DMA unmap+remap
issue, but we need to solve this in a generic way, as all drivers
would/should benefit from this optimization.


> If you still think that this shouldn't be done this way then go ahead
> and revert the patch,
> I will try to redo this as and when i find time.

Yes, please revert.

In connection with AF_XDP we need to improve/adjust the ndo_xdp_xmit
API.  We/I will try to incorporate the DMA mapping avoidance into the
API design.  Then it should hopefully be easier to redo this patch later.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

WARNING: multiple messages have this Message-ID (diff)
From: Jesper Dangaard Brouer <brouer@redhat.com>
To: Sunil Kovvuri <sunil.kovvuri@gmail.com>
Cc: "Linux Netdev List" <netdev@vger.kernel.org>,
	"Sunil Goutham" <sgoutham@cavium.com>,
	"Aleksey Makarov" <aleksey.makarov@cavium.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Christina Jacob" <cjacob@caviumnetworks.com>,
	"Daniel Borkmann" <borkmann@iogearbox.net>,
	"Alexei Starovoitov" <alexei.starovoitov@gmail.com>,
	LAKML <linux-arm-kernel@lists.infradead.org>,
	brouer@redhat.com, "Björn Töpel" <bjorn.topel@intel.com>,
	"Karlsson, Magnus" <magnus.karlsson@intel.com>
Subject: Re: [net PATCH] Revert "net: thunderx: Add support for xdp redirect"
Date: Wed, 14 Feb 2018 09:49:15 +0100	[thread overview]
Message-ID: <20180214094915.10fc6c36@redhat.com> (raw)
In-Reply-To: <CA+sq2CfYosrbZUEb0hLZS1u6tiz+m4oxEXF3-43ZxyoHq=cAVg@mail.gmail.com>

On Wed, 14 Feb 2018 01:05:16 +0530
Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:

> On Tue, Feb 13, 2018 at 10:29 PM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > This reverts commit aa136d0c82fcd6af14535853c30e219e02b2692d.
> >
> > As I previously[1] pointed out this implementation of XDP_REDIRECT is
> > wrong.  XDP_REDIRECT is a facility that must work between different
> > NIC drivers.  Another NIC driver can call ndo_xdp_xmit/nicvf_xdp_xmit,
> > but your driver patch assumes payload data (at top of page) will
> > contain a queue index and a DMA addr, this is not true and worse will
> > likely contain garbage.
> >
> > Given you have not fixed this in due time (just reached v4.16-rc1),
> > the only option I see is a revert.  
> 
> Sorry that i missed your email ie didn't respond.
>
> This driver is not for a generic PCI endpoint NIC, it's an on-silicon
> ethernet device found only on Cavium's ThunderX or OCTEONTX SOCs
> which supports upto 16 ports. XDP_REDIRECT here is mainly aimed at
> forwarding packets from one port to another
> port of same type.
> 
> The only way I could have avoided the payload data is by unmapping
> and remapping of DMA buffer which is quite expensive especially when
> IOMMU is enabled. So I thought this small optimization should be
> acceptable.

It is good that you bring up the need to avoid this DMA unmap+remap
issue, but we need to solve this in a generic way, as all drivers
would/should benefit from this optimization.


> If you still think that this shouldn't be done this way then go ahead
> and revert the patch,
> I will try to redo this as and when i find time.

Yes, please revert.

In connection with AF_XDP we need to improve/adjust the ndo_xdp_xmit
API.  We/I will try to incorporate the DMA mapping avoidance into the
API design.  Then it should hopefully be easier to redo this patch later.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

  reply	other threads:[~2018-02-14  8:49 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-13 16:59 [net PATCH] Revert "net: thunderx: Add support for xdp redirect" Jesper Dangaard Brouer
2018-02-13 16:59 ` Jesper Dangaard Brouer
2018-02-13 19:35 ` Sunil Kovvuri
2018-02-13 19:35   ` Sunil Kovvuri
2018-02-14  8:49   ` Jesper Dangaard Brouer [this message]
2018-02-14  8:49     ` Jesper Dangaard Brouer
2018-02-14 19:24 ` David Miller
2018-02-14 19:24   ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180214094915.10fc6c36@redhat.com \
    --to=brouer@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.