Message-ID: <20200708142545.66057f36@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 8 Jul 2020 14:25:45 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Michael Chan <michael.chan@...adcom.com>,
        edwin.peer@...adcom.com,
        "Tantilov, Emil S" <emil.s.tantilov@...el.com>,
        Alexander Duyck <alexander.h.duyck@...ux.intel.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Michal Kubecek <mkubecek@...e.cz>
Subject: Re: [PATCH net-next 7/9] ixgbe: convert to new udp_tunnel_nic infra

On Wed, 8 Jul 2020 10:00:36 -0700 Alexander Duyck wrote:
> On Tue, Jul 7, 2020 at 2:28 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > Make use of new common udp_tunnel_nic infra. ixgbe supports
> > IPv4 only, and only single VxLAN and Geneve ports (one each).
> >
> > I'm dropping the confusing piece of code in ixgbe_set_features().
> > ndo_udp_tunnel_add and ndo_udp_tunnel_del did not check if RXCSUM
> > is enabled, so this code was either unnecessary or buggy anyway.  
> 
> The code is unnecessary from what I can tell. I suspect the reason for
> adding the code was because the port values are used when performing
> the Rx checksum offload. However we never disable it in hardware, and
> the software path in ixgbe_rx_checksum will simply disable the related
> code anyway since we cannot set skb->encapsulation if RXCSUM is
> disabled.
> 
> With that said moving this to a separate patch might be preferable as
> it would make the patch more readable.

Ack on all points!

> > -static void ixgbe_clear_udp_tunnel_port(struct ixgbe_adapter *adapter, u32 mask)
> > +static int ixgbe_udp_tunnel_sync(struct net_device *dev, unsigned int table)
> >  {
> > +       struct ixgbe_adapter *adapter = netdev_priv(dev);
> >         struct ixgbe_hw *hw = &adapter->hw;
> > -       u32 vxlanctrl;
> > -
> > -       if (!(adapter->flags & (IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE |
> > -                               IXGBE_FLAG_GENEVE_OFFLOAD_CAPABLE)))
> > -               return;
> > +       struct udp_tunnel_info ti;
> >
> > -       vxlanctrl = IXGBE_READ_REG(hw, IXGBE_VXLANCTRL) & ~mask;
> > -       IXGBE_WRITE_REG(hw, IXGBE_VXLANCTRL, vxlanctrl);
> > -
> > -       if (mask & IXGBE_VXLANCTRL_VXLAN_UDPPORT_MASK)
> > -               adapter->vxlan_port = 0;
> > +       udp_tunnel_nic_get_port(dev, table, 0, &ti);
> > +       if (!table)
> > +               adapter->vxlan_port = ti.port;
> > +       else
> > +               adapter->geneve_port = ti.port;  
> 
> So this !table thing is a bit hard to read. It might be more useful if
> you had a define that made it clear that the expectation is that entry
> 0 is a VXLAN table and entry 1 is the GENEVE.

How about:

if (ti.type == UDP_TUNNEL_TYPE_VXLAN)

Tunnel info will have the type.

> >
> > -       if (mask & IXGBE_VXLANCTRL_GENEVE_UDPPORT_MASK)
> > -               adapter->geneve_port = 0;
> > +       IXGBE_WRITE_REG(hw, IXGBE_VXLANCTRL,
> > +                       ntohs(adapter->vxlan_port) |
> > +                       ntohs(adapter->geneve_port) <<
> > +                               IXGBE_VXLANCTRL_GENEVE_UDPPORT_SHIFT);
> > +       return 0;
> >  }  
> 
> I'm assuming the new logic will call this for all entries in the
> tables regardless of if they are set or not. If so I suppose this is
> fine.

Yup, I think this is preferred form of sync for devices which just
write registers. I wrote it for NFP initially but it seems to work 
in more places.

The driver can query the entire table and will get either the port 
or 0 if port is not set.

> >  #ifdef CONFIG_IXGBE_DCB
> >  /**
> >   * ixgbe_configure_dcb - Configure DCB hardware
> > @@ -6227,6 +6242,7 @@ static void ixgbe_init_dcb(struct ixgbe_adapter *adapter)
> >  /**
> >   * ixgbe_sw_init - Initialize general software structures (struct ixgbe_adapter)
> >   * @adapter: board private structure to initialize
> > + * @netdev: network interface device structure
> >   * @ii: pointer to ixgbe_info for device
> >   *
> >   * ixgbe_sw_init initializes the Adapter private data structure.
> > @@ -6234,6 +6250,7 @@ static void ixgbe_init_dcb(struct ixgbe_adapter *adapter)
> >   * OS network device settings (MTU size).
> >   **/
> >  static int ixgbe_sw_init(struct ixgbe_adapter *adapter,
> > +                        struct net_device *netdev,
> >                          const struct ixgbe_info *ii)
> >  {
> >         struct ixgbe_hw *hw = &adapter->hw;  
> 
> There is no need to add the argument. It should be accessible via
> adapter->netdev, or for that matter you could pass the netdev and drop
> the adapter and just pull it out via netdev_priv since the two are all
> contained in the same structure anyway. Another option would be to
> just pull the logic out of this section and put it in a switch
> statement of its own in the probe function.

Hm, new switch section definitely looks best. I'll do that.

> > @@ -6332,7 +6349,7 @@ static int ixgbe_sw_init(struct ixgbe_adapter *adapter,
> >                         adapter->flags2 |= IXGBE_FLAG2_TEMP_SENSOR_CAPABLE;
> >                 break;
> >         case ixgbe_mac_x550em_a:
> > -               adapter->flags |= IXGBE_FLAG_GENEVE_OFFLOAD_CAPABLE;
> > +               netdev->udp_tunnel_nic_info = &ixgbe_udp_tunnels_x550em_a;
> >                 switch (hw->device_id) {
> >                 case IXGBE_DEV_ID_X550EM_A_1G_T:
> >                 case IXGBE_DEV_ID_X550EM_A_1G_T_L:
> > @@ -6359,7 +6376,8 @@ static int ixgbe_sw_init(struct ixgbe_adapter *adapter,
> >  #ifdef CONFIG_IXGBE_DCA
> >                 adapter->flags &= ~IXGBE_FLAG_DCA_CAPABLE;
> >  #endif
> > -               adapter->flags |= IXGBE_FLAG_VXLAN_OFFLOAD_CAPABLE;
> > +               if (!netdev->udp_tunnel_nic_info)
> > +                       netdev->udp_tunnel_nic_info = &ixgbe_udp_tunnels_x550;
> >                 break;
> >         default:
> >                 break;  
> 
> Not a fan of the !udp_tunnel_nic_info check, but I understand you are
> having to do it because of the fall-through.

Yup.

> > @@ -6798,8 +6816,7 @@ int ixgbe_open(struct net_device *netdev)
> >
> >         ixgbe_up_complete(adapter);
> >
> > -       ixgbe_clear_udp_tunnel_port(adapter, IXGBE_VXLANCTRL_ALL_UDPPORT_MASK);
> > -       udp_tunnel_get_rx_info(netdev);
> > +       udp_tunnel_nic_reset_ntf(netdev);
> >
> >         return 0;  
> 
> So I went looking through the earlier patches for an explanation of
> udp_tunnel_nic_reset_ntf. It might be useful to have some doxygen
> comments with the declaration explaining what it does and when/why we
> should use it.

Looks like I hated the need for this one so much I forgot to document
it :S

I was hoping the core can issue all the callbacks just based on netdev
notifications, but it seems like most drivers have internal resets
procedures which require re-programming the ports :( Hence this helper
was born.

I'll document it like this:

 * Called by the driver to inform the core that the entire UDP tunnel port
 * state has been lost, usually due to device reset. Core will assume device
 * forgot all the ports and issue .set_port and .sync_table callbacks as
 * necessary.


Thanks for the review! I'll post v2 in a couple hours hoping someone
else will find time to review :S

Powered by blists - more mailing lists