Jarod Wilson <ja...@redhat.com> wrote:
>Currently, this support is limited to active-backup mode, as I'm not sure
>about the feasilibity of mapping an xfrm_state's offload handle to
>multiple hardware devices simultaneously, and we rely on being able to
>pass some hints to both the xfrm and NIC driver about whether or not
>they're operating on a slave device.
>
>I've tested this atop an Intel x520 device (ixgbe) using libreswan in
>transport mode, succesfully achieving ~4.3Gbps throughput with netperf
>(more or less identical to throughput on a bare NIC in this system),
>as well as successful failover and recovery mid-netperf.
>
>v2: rebase on latest net-next and wrap with #ifdef CONFIG_XFRM_OFFLOAD
>v3: add new CONFIG_BOND_XFRM_OFFLOAD option and fix shutdown path
>
>CC: Jay Vosburgh <j.vosbu...@gmail.com>
>CC: Veaceslav Falico <vfal...@gmail.com>
>CC: Andy Gospodarek <a...@greyhouse.net>
>CC: "David S. Miller" <da...@davemloft.net>
>CC: Jeff Kirsher <jeffrey.t.kirs...@intel.com>
>CC: Jakub Kicinski <k...@kernel.org>
>CC: Steffen Klassert <steffen.klass...@secunet.com>
>CC: Herbert Xu <herb...@gondor.apana.org.au>
>CC: net...@vger.kernel.org
>CC: intel-wired-...@lists.osuosl.org
>Signed-off-by: Jarod Wilson <ja...@redhat.com>
>
>Signed-off-by: Jarod Wilson <ja...@redhat.com>
>---
> drivers/net/Kconfig | 11 ++++
> drivers/net/bonding/bond_main.c | 111 +++++++++++++++++++++++++++++++-
> include/net/bonding.h | 3 +
> 3 files changed, 122 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>index c7d310ef1c83..938c4dd9bfb9 100644
>--- a/drivers/net/Kconfig
>+++ b/drivers/net/Kconfig
>@@ -56,6 +56,17 @@ config BONDING
> To compile this driver as a module, choose M here: the module
> will be called bonding.
>
>+config BONDING_XFRM_OFFLOAD
>+ bool "Bonding driver IPSec XFRM cryptography-offload pass-through
>support"
>+ depends on BONDING
>+ depends on XFRM_OFFLOAD
>+ default y
>+ select XFRM_ALGO
>+ ---help---
>+ Enable support for IPSec offload pass-through in the bonding driver.
>+ Currently limited to active-backup mode only, and requires slave
>+ devices that support hardware crypto offload.
>+
Why is this a separate Kconfig option? Is it reasonable to
expect users to enable XFRM_OFFLOAD but not BONDING_XFRM_OFFLOAD?
> config DUMMY
> tristate "Dummy net driver support"
> ---help---
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index a25c65d4af71..01b80cef492a 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -79,6 +79,7 @@
> #include <net/pkt_sched.h>
> #include <linux/rculist.h>
> #include <net/flow_dissector.h>
>+#include <net/xfrm.h>
> #include <net/bonding.h>
> #include <net/bond_3ad.h>
> #include <net/bond_alb.h>
>@@ -278,8 +279,6 @@ const char *bond_mode_name(int mode)
> return names[mode];
> }
>
>-/*---------------------------------- VLAN
>-----------------------------------*/
>-
> /**
> * bond_dev_queue_xmit - Prepare skb for xmit.
> *
>@@ -302,6 +301,8 @@ netdev_tx_t bond_dev_queue_xmit(struct bonding *bond,
>struct sk_buff *skb,
> return dev_queue_xmit(skb);
> }
>
>+/*---------------------------------- VLAN
>-----------------------------------*/
>+
> /* In the following 2 functions, bond_vlan_rx_add_vid and
> bond_vlan_rx_kill_vid,
> * We don't protect the slave list iteration with a lock because:
> * a. This operation is performed in IOCTL context,
>@@ -372,6 +373,84 @@ static int bond_vlan_rx_kill_vid(struct net_device
>*bond_dev,
> return 0;
> }
>
>+/*---------------------------------- XFRM
>-----------------------------------*/
>+
>+#ifdef CONFIG_BONDING_XFRM_OFFLOAD
>+/**
>+ * bond_ipsec_add_sa - program device with a security association
>+ * @xs: pointer to transformer state struct
>+ **/
>+static int bond_ipsec_add_sa(struct xfrm_state *xs)
>+{
>+ struct net_device *bond_dev = xs->xso.dev;
>+ struct bonding *bond = netdev_priv(bond_dev);
>+ struct slave *slave = rtnl_dereference(bond->curr_active_slave);
>+
>+ xs->xso.slave_dev = slave->dev;
>+ bond->xs = xs;
>+
>+ if (!(slave->dev->xfrmdev_ops
>+ && slave->dev->xfrmdev_ops->xdo_dev_state_add)) {
>+ slave_warn(bond_dev, slave->dev, "Slave does not support ipsec
>offload\n");
>+ return -EINVAL;
>+ }
>+
>+ return slave->dev->xfrmdev_ops->xdo_dev_state_add(xs);
>+}
>+
>+/**
>+ * bond_ipsec_del_sa - clear out this specific SA
>+ * @xs: pointer to transformer state struct
>+ **/
>+static void bond_ipsec_del_sa(struct xfrm_state *xs)
>+{
>+ struct net_device *bond_dev = xs->xso.dev;
>+ struct bonding *bond = netdev_priv(bond_dev);
>+ struct slave *slave = rtnl_dereference(bond->curr_active_slave);
>+
>+ if (!slave)
>+ return;
>+
>+ xs->xso.slave_dev = slave->dev;
>+
>+ if (!(slave->dev->xfrmdev_ops
>+ && slave->dev->xfrmdev_ops->xdo_dev_state_delete)) {
>+ slave_warn(bond_dev, slave->dev, "%s: no slave
>xdo_dev_state_delete\n", __func__);
>+ return;
>+ }
>+
>+ slave->dev->xfrmdev_ops->xdo_dev_state_delete(xs);
>+}
>+
>+/**
>+ * bond_ipsec_offload_ok - can this packet use the xfrm hw offload
>+ * @skb: current data packet
>+ * @xs: pointer to transformer state struct
>+ **/
>+static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>+{
>+ struct net_device *bond_dev = xs->xso.dev;
>+ struct bonding *bond = netdev_priv(bond_dev);
>+ struct slave *curr_active = rtnl_dereference(bond->curr_active_slave);
>+ struct net_device *slave_dev = curr_active->dev;
>+
>+ if (!(slave_dev->xfrmdev_ops
>+ && slave_dev->xfrmdev_ops->xdo_dev_offload_ok)) {
>+ slave_warn(bond_dev, slave_dev, "%s: no slave
>xdo_dev_offload_ok\n", __func__);
>+ return false;
>+ }
>+
>+ xs->xso.slave_dev = slave_dev;
>+ return slave_dev->xfrmdev_ops->xdo_dev_offload_ok(skb, xs);
>+}
>+
>+static const struct xfrmdev_ops bond_xfrmdev_ops = {
>+ .xdo_dev_state_add = bond_ipsec_add_sa,
>+ .xdo_dev_state_delete = bond_ipsec_del_sa,
>+ .xdo_dev_offload_ok = bond_ipsec_offload_ok,
>+};
>+#endif /* CONFIG_BONDING_XFRM_OFFLOAD */
>+
> /*------------------------------- Link status
> -------------------------------*/
>
> /* Set the carrier state for the master according to the state of its
>@@ -879,6 +958,11 @@ void bond_change_active_slave(struct bonding *bond,
>struct slave *new_active)
> return;
>
> if (new_active) {
>+#ifdef CONFIG_BONDING_XFRM_OFFLOAD
>+ if ((BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) && bond->xs)
>+ bond_ipsec_del_sa(bond->xs);
>+#endif /* CONFIG_BONDING_XFRM_OFFLOAD */
>+
> new_active->last_link_up = jiffies;
>
> if (new_active->link == BOND_LINK_BACK) {
>@@ -941,6 +1025,13 @@ void bond_change_active_slave(struct bonding *bond,
>struct slave *new_active)
> bond_should_notify_peers(bond);
> }
>
>+#ifdef CONFIG_BONDING_XFRM_OFFLOAD
>+ if (old_active && bond->xs) {
>+ xfrm_dev_state_flush(dev_net(bond->dev),
>bond->dev, true);
>+ bond_ipsec_add_sa(bond->xs);
>+ }
>+#endif /* CONFIG_BONDING_XFRM_OFFLOAD */
>+
> call_netdevice_notifiers(NETDEV_BONDING_FAILOVER,
> bond->dev);
> if (should_notify_peers) {
> bond->send_peer_notif--;
>@@ -1125,7 +1216,9 @@ static netdev_features_t bond_fix_features(struct
>net_device *dev,
> NETIF_F_HIGHDMA | NETIF_F_LRO)
>
> #define BOND_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
>- NETIF_F_RXCSUM | NETIF_F_ALL_TSO)
>+ NETIF_F_RXCSUM | NETIF_F_ALL_TSO | \
>+ NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
>+ NETIF_F_GSO_ESP)
>
> #define BOND_MPLS_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
> NETIF_F_ALL_TSO)
>@@ -1464,6 +1557,9 @@ int bond_enslave(struct net_device *bond_dev, struct
>net_device *slave_dev,
> slave_dbg(bond_dev, slave_dev, "is !NETIF_F_VLAN_CHALLENGED\n");
> }
>
>+ if (slave_dev->features & NETIF_F_HW_ESP)
>+ slave_dbg(bond_dev, slave_dev, "is esp-hw-offload capable\n");
>+
> /* Old ifenslave binaries are no longer supported. These can
> * be identified with moderate accuracy by the state of the slave:
> * the current ifenslave will set the interface down prior to
>@@ -4542,6 +4638,13 @@ void bond_setup(struct net_device *bond_dev)
> bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT | IFF_NO_QUEUE;
> bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>
>+#ifdef CONFIG_BONDING_XFRM_OFFLOAD
>+ /* set up xfrm device ops (only supported in active-backup right now) */
>+ if ((BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP))
>+ bond_dev->xfrmdev_ops = &bond_xfrmdev_ops;
>+ bond->xs = NULL;
>+#endif /* CONFIG_BONDING_XFRM_OFFLOAD */
>+
> /* don't acquire bond device's netif_tx_lock when transmitting */
> bond_dev->features |= NETIF_F_LLTX;
>
>@@ -4560,6 +4663,8 @@ void bond_setup(struct net_device *bond_dev)
> NETIF_F_HW_VLAN_CTAG_FILTER;
>
> bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
>+ if ((BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP))
>+ bond_dev->hw_features |= BOND_ENC_FEATURES;
Why is adding the ESP features to hw_features (here, and added
to BOND_ENC_FEATURES, above) not behind CONFIG_BONDING_XFRM_OFFLOAD?
If adding these features makes sense regardless of the
XFRM_OFFLOAD configuration, then shouldn't this change to feature
handling be a separate patch? The feature handling is complex, and is
worth its own patch so it stands out in the log.
-J
> bond_dev->features |= bond_dev->hw_features;
> bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
> }
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index aa854a9c01e2..29a25098e2a6 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -238,6 +238,9 @@ struct bonding {
> struct dentry *debug_dir;
> #endif /* CONFIG_DEBUG_FS */
> struct rtnl_link_stats64 bond_stats;
>+#ifdef CONFIG_BONDING_XFRM_OFFLOAD
>+ struct xfrm_state *xs;
>+#endif /* CONFIG_BONDING_XFRM_OFFLOAD */
> };
>
> #define bond_slave_get_rcu(dev) \
>--
>2.20.1
>
---
-Jay Vosburgh, jay.vosbu...@canonical.com
