Re: [PATCH V4 mlx5-next 10/15] RDMA/core: Add LAG functionality

Jason Gunthorpe
2020-04-22T12:50:38Z

On Wed, Apr 22, 2020 at 11:39:46AM +0300, Maor Gottlieb wrote:
> Add support to get the RoCE LAG xmit slave by building skb
> of the RoCE packet and call to master_get_xmit_slave.
> If driver wants to get the slave assume all slaves are available,
> then need to set RDMA_LAG_FLAGS_HASH_ALL_SLAVES in flags.
>
> Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/Makefile | 2 +-
> drivers/infiniband/core/lag.c | 138 +++++++++++++++++++++++++++++++
> include/rdma/ib_verbs.h | 2 +
> include/rdma/lag.h | 22 +++++
> 4 files changed, 163 insertions(+), 1 deletion(-)
> create mode 100644 drivers/infiniband/core/lag.c
> create mode 100644 include/rdma/lag.h
>
> diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
> index d1b14887960e..870f0fcd54d5 100644
> +++ b/drivers/infiniband/core/Makefile
> @@ -12,7 +12,7 @@ ib_core-y := packer.o ud_header.o verbs.o cq.o rw.o sysfs.o \
> roce_gid_mgmt.o mr_pool.o addr.o sa_query.o \
> multicast.o mad.o smi.o agent.o mad_rmpp.o \
> nldev.o restrack.o counters.o ib_core_uverbs.o \
> - trace.o
> + trace.o lag.o
>
> ib_core-$(CONFIG_SECURITY_INFINIBAND) += security.o
> ib_core-$(CONFIG_CGROUP_RDMA) += cgroup.o
> diff --git a/drivers/infiniband/core/lag.c b/drivers/infiniband/core/lag.c
> new file mode 100644
> index 000000000000..3036fb3dc43a
> +++ b/drivers/infiniband/core/lag.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * Copyright (c) 2020 Mellanox Technologies. All rights reserved.
> + */
> +
> +#include <rdma/ib_verbs.h>
> +#include <rdma/ib_cache.h>
> +#include <rdma/lag.h>
> +
> +static struct sk_buff *rdma_build_skb(struct ib_device *device,
> + struct net_device *netdev,
> + struct rdma_ah_attr *ah_attr)
> +{
> + struct ipv6hdr *ip6h;
> + struct sk_buff *skb;
> + struct ethhdr *eth;
> + struct iphdr *iph;
> + struct udphdr *uh;
> + u8 smac[ETH_ALEN];
> + bool is_ipv4;
> + int hdr_len;
> +
> + is_ipv4 = ipv6_addr_v4mapped((struct in6_addr *)ah_attr->grh.dgid.raw);
> + hdr_len = ETH_HLEN + sizeof(struct udphdr) + LL_RESERVED_SPACE(netdev);
> + hdr_len += is_ipv4 ? sizeof(struct iphdr) : sizeof(struct ipv6hdr);
> +
> + skb = alloc_skb(hdr_len, GFP_ATOMIC);
> + if (!skb)
> + return NULL;
> +
> + skb->dev = netdev;
> + skb_reserve(skb, hdr_len);
> + skb_push(skb, sizeof(struct udphdr));
> + skb_reset_transport_header(skb);
> + uh = udp_hdr(skb);
> + uh->source = htons(0xC000);
> + uh->dest = htons(ROCE_V2_UDP_DPORT);
> + uh->len = htons(sizeof(struct udphdr));
> +
> + if (is_ipv4) {
> + skb_push(skb, sizeof(struct iphdr));
> + skb_reset_network_header(skb);
> + iph = ip_hdr(skb);
> + iph->frag_off = 0;
> + iph->version = 4;
> + iph->protocol = IPPROTO_UDP;
> + iph->ihl = 0x5;
> + iph->tot_len = htons(sizeof(struct udphdr) + sizeof(struct
> + iphdr));
> + memcpy(&iph->saddr, ah_attr->grh.sgid_attr->gid.raw + 12,
> + sizeof(struct in_addr));
> + memcpy(&iph->daddr, ah_attr->grh.dgid.raw + 12,
> + sizeof(struct in_addr));
> + } else {
> + skb_push(skb, sizeof(struct ipv6hdr));
> + skb_reset_network_header(skb);
> + ip6h = ipv6_hdr(skb);
> + ip6h->version = 6;
> + ip6h->nexthdr = IPPROTO_UDP;
> + memcpy(&ip6h->flow_lbl, &ah_attr->grh.flow_label,
> + sizeof(*ip6h->flow_lbl));
> + memcpy(&ip6h->saddr, ah_attr->grh.sgid_attr->gid.raw,
> + sizeof(struct in6_addr));
> + memcpy(&ip6h->daddr, ah_attr->grh.dgid.raw,
> + sizeof(struct in6_addr));
> + }

What about setting up the UDP header? It looks like this needs to be
before the sport patch and the sport patch needs to modify here too.

> +void rdma_lag_put_ah_roce_slave(struct rdma_ah_attr *ah_attr)
> +{
> + if (ah_attr->roce.xmit_slave)
> + dev_put(ah_attr->roce.xmit_slave);
> +}
> +
> +int rdma_lag_get_ah_roce_slave(struct ib_device *device,
> + struct rdma_ah_attr *ah_attr)
> +{
> + struct net_device *master;
> + struct net_device *slave;
> +
> + if (!(ah_attr->type == RDMA_AH_ATTR_TYPE_ROCE &&
> + ah_attr->grh.sgid_attr->gid_type == IB_GID_TYPE_ROCE_UDP_ENCAP))
> + return 0;
> +
> + rcu_read_lock();
> + master = rdma_read_gid_attr_ndev_rcu(ah_attr->grh.sgid_attr);
> + if (IS_ERR(master)) {
> + rcu_read_unlock();
> + return PTR_ERR(master);
> + }
> + dev_hold(master);
> + rcu_read_unlock();
> +
> + if (!netif_is_bond_master(master)) {
> + dev_put(master);
> + return 0;
> + }
> +
> + slave = rdma_get_xmit_slave_udp(device, master, ah_attr);
> +
> + dev_put(master);
> + if (!slave) {
> + ibdev_warn(device, "Failed to get lag xmit slave\n");
> + return -EINVAL;
> + }
> +
> + ah_attr->roce.xmit_slave = slave;

Is xmit_slave is reliably NULL in the other return 0 cases?

Jason