From: Jakub Kicinski <kuba@kernel.org>
To: Alexander Duyck <alexander.duyck@gmail.com>
Cc: David Miller <davem@davemloft.net>,
Netdev <netdev@vger.kernel.org>,
Saeed Mahameed <saeedm@mellanox.com>,
Michael Chan <michael.chan@broadcom.com>,
edwin.peer@broadcom.com, "Tantilov,
Emil S" <emil.s.tantilov@intel.com>,
Alexander Duyck <alexander.h.duyck@linux.intel.com>,
Jeff Kirsher <jeffrey.t.kirsher@intel.com>,
Tariq Toukan <tariqt@mellanox.com>,
Michal Kubecek <mkubecek@suse.cz>
Subject: Re: [PATCH net-next 7/9] ixgbe: convert to new udp_tunnel_nic infra
Date: Wed, 8 Jul 2020 14:25:45 -0700 [thread overview]
Message-ID: <20200708142545.66057f36@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <CAKgT0Udi60-zs08eppC-5roPnpi_r4nAV53xovG8xfnovUtS4A@mail.gmail.com>
On Wed, 8 Jul 2020 10:00:36 -0700 Alexander Duyck wrote:
> On Tue, Jul 7, 2020 at 2:28 PM Jakub Kicinski <kuba@kernel.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
next prev parent reply other threads:[~2020-07-08 21:25 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-07 21:24 [PATCH net-next 0/9] udp_tunnel: add NIC RX port offload infrastructure Jakub Kicinski
2020-07-07 21:24 ` [PATCH net-next 1/9] debugfs: make sure we can remove u32_array files cleanly Jakub Kicinski
2020-07-08 7:22 ` Greg Kroah-Hartman
2020-07-07 21:24 ` [PATCH net-next 2/9] udp_tunnel: re-number the offload tunnel types Jakub Kicinski
2020-07-07 21:24 ` [PATCH net-next 3/9] udp_tunnel: add central NIC RX port offload infrastructure Jakub Kicinski
2020-07-07 21:24 ` [PATCH net-next 4/9] ethtool: add tunnel info interface Jakub Kicinski
2020-07-08 22:32 ` Michal Kubecek
2020-07-08 23:30 ` Jakub Kicinski
2020-07-09 0:03 ` Michal Kubecek
2020-07-07 21:24 ` [PATCH net-next 5/9] netdevsim: add UDP tunnel port offload support Jakub Kicinski
2020-07-07 21:24 ` [PATCH net-next 6/9] selftests: net: add a test for UDP tunnel info infra Jakub Kicinski
2020-07-07 21:24 ` [PATCH net-next 7/9] ixgbe: convert to new udp_tunnel_nic infra Jakub Kicinski
2020-07-08 17:00 ` Alexander Duyck
2020-07-08 21:25 ` Jakub Kicinski [this message]
2020-07-08 23:14 ` Alexander Duyck
2020-07-07 21:24 ` [PATCH net-next 8/9] bnxt: " Jakub Kicinski
2020-07-07 21:24 ` [PATCH net-next 9/9] mlx4: " Jakub Kicinski
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=20200708142545.66057f36@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
--to=kuba@kernel.org \
--cc=alexander.duyck@gmail.com \
--cc=alexander.h.duyck@linux.intel.com \
--cc=davem@davemloft.net \
--cc=edwin.peer@broadcom.com \
--cc=emil.s.tantilov@intel.com \
--cc=jeffrey.t.kirsher@intel.com \
--cc=michael.chan@broadcom.com \
--cc=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=saeedm@mellanox.com \
--cc=tariqt@mellanox.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).