[Intel-wired-lan] [PATCH v6 1/2] i40e: add XDP support for pass and drop actions

[Duyck, Alexander H]

On Tue, 2017-05-23 at 20:51 +0200, Björn Töpel wrote:
> 2017-05-23 18:51 GMT+02:00 Alexander Duyck <alexander.duyck at gmail.com>:
> > 
> > On Tue, May 23, 2017 at 12:33 AM, Björn Töpel <bjorn.topel at gmail.com> wrote:
> > > 
> > > From: Björn Töpel <bjorn.topel at intel.com>
> > > 
> > > This commit adds basic XDP support for i40e derived NICs. All XDP
> > > actions will end up in XDP_DROP.
> > > 
> > > Signed-off-by: Björn Töpel <bjorn.topel at intel.com>
> > 
> > So I only really see one issue which I pointed out earlier. Basically
> > the i40e_change_mtu call can't really be dependent on vsi->rx_buf_len
> > since rx_buf_len is changed as a result of changing the MTU.
> > 
> > > 
> > > ---
> > >  drivers/net/ethernet/intel/i40e/i40e.h      |   7 ++
> > >  drivers/net/ethernet/intel/i40e/i40e_main.c |  75 ++++++++++++++++
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 130 +++++++++++++++++++++-------
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.h |   1 +
> > >  4 files changed, 182 insertions(+), 31 deletions(-)
> > > 
> > 
> > [...]
> > 
> > > 
> > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > > index 8d1d3b859af7..c8b1db0ebb9e 100644
> > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > > @@ -27,6 +27,7 @@
> > >  #include <linux/etherdevice.h>
> > >  #include <linux/of_net.h>
> > >  #include <linux/pci.h>
> > > +#include <linux/bpf.h>
> > > 
> > >  /* Local includes */
> > >  #include "i40e.h"
> > > @@ -2408,6 +2409,13 @@ static int i40e_change_mtu(struct net_device *netdev, int new_mtu)
> > >         struct i40e_vsi *vsi = np->vsi;
> > >         struct i40e_pf *pf = vsi->back;
> > > 
> > > +       if (i40e_enabled_xdp_vsi(vsi)) {
> > > +               int frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> > > +
> > > +               if (frame_size > vsi->rx_buf_len)
> > > +                       return -EINVAL;
> > > +       }
> > > +
> > 
> > So this code suffers from the same issue that John's ixgbe code did.
> > You might be better off implementing something like we did with
> > i40e_vsi_configure_rx. Basically the upper limit can be either 3K or
> > 2K if the page size greater than 4K or the LEGACY_RX flag is set. You
> > might look at adding a check here for that instead of just comparing
> > it to vsi->rx_buf_len since rx_buf_len can change depending on the MTU
> > size.
> > 
> 
> You pointed this out in our private conversation, but obviously I
> didn't get it... :-(
> 
> So, something in lines of:
> 
> @@ -2396,6 +2397,18 @@ static void i40e_sync_filters_subtask(struct i40e_pf *pf)
>  }
> 
>  /**
> + * i40e_max_xdp_frame_size - returns the maximum allowed frame size for XDP
> + * @vsi: the vsi
> + **/
> +static int i40e_max_xdp_frame_size(struct i40e_vsi *vsi)
> +{
> +       if (PAGE_SIZE >= 8192 || (vsi->back->flags & I40E_FLAG_LEGACY_RX))
> +               return I40E_RXBUFFER_2048;
> +       else
> +               return I40E_RXBUFFER_3072;
> +}
> +
> +/**
>   * i40e_change_mtu - NDO callback to change the Maximum Transfer Unit
>   * @netdev: network interface device structure
>   * @new_mtu: new value for maximum frame size
> @@ -2408,6 +2421,13 @@ static int i40e_change_mtu(struct net_device
> *netdev, int new_mtu)
>         struct i40e_vsi *vsi = np->vsi;
>         struct i40e_pf *pf = vsi->back;
> 
> +       if (i40e_enabled_xdp_vsi(vsi)) {
> +               int frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> +
> +               if (frame_size > i40e_max_xdp_frame_size(vsi))
> +                       return -EINVAL;
> +       }
> +
>         netdev_info(netdev, "changing MTU from %d to %d\n",
>                     netdev->mtu, new_mtu);
>         netdev->mtu = new_mtu;
> 
> 
> 
> Björn

Yes this is exactly what I had in mind. If you can fold this into your
patch then I would say we pretty much have this all wrapped up (at
least until we find something in testing :-)).

Thanks.

- Alex