mlx4: Add support for IBoE - address resolution
PATCH v10 8/12 mlx4: Add support for IBoE - address resolution
The following patch handles address vectors creation for IBoE ports. mlx4 needs the MAC address of the remote node to include it in the WQE of a UD QP or in the QP context of connected QPs. Address resolution is done atomically in the case of a link local address or a multicast GID and otherwise -EINVAL is returned. mlx4 transport packets were changed too to accommodate for IBoE. Multicast groups attach/detach calls dev_mc_add/remove to update the NIC's multicast filters.Since attaching a QP to a multicast group does not require the QP to be in a state different then INIT - this is fine for IB. For IBoE however, we need the port assigned to the QP in order to call dev_mc_add() for the correct netdevice, while port is assigned when moving from INIT to RTR. Hence, we must keep track of all the multicast groups attached to a QP and call dev_mc_add() when the port becomes available.
---
Changes form v9:
Cosmentic change to a conditon that checks the link layer.
drivers/infiniband/hw/mlx4/ah.c | 161 +++++++++--- drivers/infiniband/hw/mlx4/mad.c | 32 ++- drivers/infiniband/hw/mlx4/main.c | 474 +++++++++++++++++++++++++++++++--- drivers/infiniband/hw/mlx4/mlx4_ib.h | 32 +++- drivers/infiniband/hw/mlx4/qp.c | 140 ++++++++-- drivers/net/mlx4/en_port.c | 4 +- drivers/net/mlx4/en_port.h | 3 +- drivers/net/mlx4/fw.c | 3 +- include/linux/mlx4/cmd.h | 1 + include/linux/mlx4/device.h | 30 ++- include/linux/mlx4/qp.h | 7 +- 11 files changed, 771 insertions(+), 116 deletions(-)
drivers/infiniband/hw/mlx4/ah.c +128 -33
@@ -33,63 +33,157 @@#include <linux/slab.h> #include "mlx4_ib.h" +#include <rdma/ib_addr.h> +#include <linux/inet.h> +#include <linux/string.h> -struct ib_ah *mlx4_ib_create_ah (struct ib_pd *pd,struct ib_ah_attr *ah_attr) +int mlx4_ib_resolve_grh (struct mlx4_ib_dev *dev,const struct ib_ah_attr *ah_attr, +u8 *mac,int *is_mcast,u8 port) { -struct mlx4_dev *dev =to_mdev (pd->device)->dev; -struct mlx4_ib_ah *ah; +struct mlx4_ib_iboe *iboe = &dev->iboe; +struct in6_addr in6; - ah =kmalloc (sizeof *ah,GFP_ATOMIC ); -if (!ah) -return ERR_PTR (-ENOMEM ); + *is_mcast =0 ; +spin_lock (&iboe->lock); +if (!iboe->netdevs[port -1 ]) { +spin_unlock (&iboe->lock); +return -EINVAL ; + } +spin_unlock (&iboe->lock); -memset (&ah->av,0 ,sizeof ah->av); +memcpy (&in6, ah_attr->grh.dgid.raw,sizeof in6); +if (rdma_link_local_addr (&in6)) +rdma_get_ll_mac (&in6, mac); +else if (rdma_is_multicast_addr (&in6)) { +rdma_get_mcast_mac (&in6, mac); + *is_mcast =1 ; + }else +return -EINVAL ; - ah->av.port_pd =cpu_to_be32 (to_mpd (pd)->pdn | (ah_attr->port_num <<24 )); - ah->av.g_slid = ah_attr->src_path_bits; - ah->av.dlid =cpu_to_be16 (ah_attr->dlid); -if (ah_attr->static_rate) { - ah->av.stat_rate = ah_attr->static_rate +MLX4_STAT_RATE_OFFSET ; -while (ah->av.stat_rate >IB_RATE_2_5_GBPS +MLX4_STAT_RATE_OFFSET && - !(1 << ah->av.stat_rate & dev->caps.stat_rate_support)) - --ah->av.stat_rate; - } - ah->av.sl_tclass_flowlabel =cpu_to_be32 (ah_attr->sl <<28 ); +return 0 ; +} + +static struct ib_ah *create_ib_ah (struct ib_pd *pd,struct ib_ah_attr *ah_attr, +struct mlx4_ib_ah *ah) +{ +struct mlx4_dev *dev =to_mdev (pd->device)->dev; + + ah->av.ib.port_pd =cpu_to_be32 (to_mpd (pd)->pdn | (ah_attr->port_num <<24 )); + ah->av.ib.g_slid = ah_attr->src_path_bits;if (ah_attr->ah_flags &IB_AH_GRH ) { - ah->av.g_slid |=0x80 ; - ah->av.gid_index = ah_attr->grh.sgid_index; - ah->av.hop_limit = ah_attr->grh.hop_limit; - ah->av.sl_tclass_flowlabel |= + ah->av.ib.g_slid |=0x80 ; + ah->av.ib.gid_index = ah_attr->grh.sgid_index; + ah->av.ib.hop_limit = ah_attr->grh.hop_limit; + ah->av.ib.sl_tclass_flowlabel |=cpu_to_be32 ((ah_attr->grh.traffic_class <<20 ) | ah_attr->grh.flow_label); -memcpy (ah->av.dgid, ah_attr->grh.dgid.raw,16 ); +memcpy (ah->av.ib.dgid, ah_attr->grh.dgid.raw,16 ); + } + + ah->av.ib.dlid =cpu_to_be16 (ah_attr->dlid); +if (ah_attr->static_rate) { + ah->av.ib.stat_rate = ah_attr->static_rate +MLX4_STAT_RATE_OFFSET ; +while (ah->av.ib.stat_rate >IB_RATE_2_5_GBPS +MLX4_STAT_RATE_OFFSET && + !(1 << ah->av.ib.stat_rate & dev->caps.stat_rate_support)) + --ah->av.ib.stat_rate; } + ah->av.ib.sl_tclass_flowlabel =cpu_to_be32 (ah_attr->sl <<28 );return &ah->ibah; } +static struct ib_ah *create_iboe_ah (struct ib_pd *pd,struct ib_ah_attr *ah_attr, +struct mlx4_ib_ah *ah) +{ +struct mlx4_ib_dev *ibdev =to_mdev (pd->device); +struct mlx4_dev *dev = ibdev->dev; +u8 mac[6 ]; +int err; +int is_mcast; + + err =mlx4_ib_resolve_grh (ibdev, ah_attr, mac, &is_mcast, ah_attr->port_num); +if (err) +return ERR_PTR (err); + +memcpy (ah->av.eth.mac, mac,6 ); + ah->av.eth.port_pd =cpu_to_be32 (to_mpd (pd)->pdn | (ah_attr->port_num <<24 )); + ah->av.eth.gid_index = ah_attr->grh.sgid_index; +if (ah_attr->static_rate) { + ah->av.eth.stat_rate = ah_attr->static_rate +MLX4_STAT_RATE_OFFSET ; +while (ah->av.eth.stat_rate >IB_RATE_2_5_GBPS +MLX4_STAT_RATE_OFFSET && + !(1 << ah->av.eth.stat_rate & dev->caps.stat_rate_support)) + --ah->av.eth.stat_rate; + } + +/* +* HW requires multicast LID so we just choose one. +*/ +if (is_mcast) + ah->av.ib.dlid =cpu_to_be16 (0xc000 ); + +memcpy (ah->av.eth.dgid, ah_attr->grh.dgid.raw,16 ); + ah->av.eth.sl_tclass_flowlabel =cpu_to_be32 (ah_attr->sl <<28 ); + +return &ah->ibah; +} + +struct ib_ah *mlx4_ib_create_ah (struct ib_pd *pd,struct ib_ah_attr *ah_attr) +{ +struct mlx4_ib_ah *ah; +struct ib_ah *ret; + + ah =kzalloc (sizeof *ah,GFP_ATOMIC ); +if (!ah) +return ERR_PTR (-ENOMEM ); + +if (rdma_port_get_link_layer (pd->device, ah_attr->port_num) ==IB_LINK_LAYER_ETHERNET ) { +if (!(ah_attr->ah_flags &IB_AH_GRH )) { + ret =ERR_PTR (-EINVAL ); + goto out; + }else { +/* TBD: need to handle the case when we get called +in an atomic context and there we might sleep. We +don't expect this currently since we're working with +link local addresses which we can translate without +going to sleep */ + ret =create_iboe_ah (pd, ah_attr, ah); +if (IS_ERR (ret)) + goto out; +else +return ret; + } + }else +return create_ib_ah (pd, ah_attr, ah);/* never fails */ + +out: +kfree (ah); +return ret; +} +int mlx4_ib_query_ah (struct ib_ah *ibah,struct ib_ah_attr *ah_attr) {struct mlx4_ib_ah *ah =to_mah (ibah); +enum rdma_link_layer ll;memset (ah_attr,0 ,sizeof *ah_attr); - ah_attr->dlid =be16_to_cpu (ah->av.dlid); - ah_attr->sl =be32_to_cpu (ah->av.sl_tclass_flowlabel) >>28 ; - ah_attr->port_num =be32_to_cpu (ah->av.port_pd) >>24 ; -if (ah->av.stat_rate) - ah_attr->static_rate = ah->av.stat_rate -MLX4_STAT_RATE_OFFSET ; - ah_attr->src_path_bits = ah->av.g_slid &0x7F ; + ah_attr->sl =be32_to_cpu (ah->av.ib.sl_tclass_flowlabel) >>28 ; + ah_attr->port_num =be32_to_cpu (ah->av.ib.port_pd) >>24 ; + ll =rdma_port_get_link_layer (ibah->device, ah_attr->port_num); + ah_attr->dlid = ll ==IB_LINK_LAYER_INFINIBAND ?be16_to_cpu (ah->av.ib.dlid) :0 ; +if (ah->av.ib.stat_rate) + ah_attr->static_rate = ah->av.ib.stat_rate -MLX4_STAT_RATE_OFFSET ; + ah_attr->src_path_bits = ah->av.ib.g_slid &0x7F ;if (mlx4_ib_ah_grh_present (ah)) { ah_attr->ah_flags =IB_AH_GRH ; ah_attr->grh.traffic_class = -be32_to_cpu (ah->av.sl_tclass_flowlabel) >>20 ; +be32_to_cpu (ah->av.ib.sl_tclass_flowlabel) >>20 ; ah_attr->grh.flow_label = -be32_to_cpu (ah->av.sl_tclass_flowlabel) &0xfffff ; - ah_attr->grh.hop_limit = ah->av.hop_limit; - ah_attr->grh.sgid_index = ah->av.gid_index; -memcpy (ah_attr->grh.dgid.raw, ah->av.dgid,16 ); +be32_to_cpu (ah->av.ib.sl_tclass_flowlabel) &0xfffff ; + ah_attr->grh.hop_limit = ah->av.ib.hop_limit; + ah_attr->grh.sgid_index = ah->av.ib.gid_index; +memcpy (ah_attr->grh.dgid.raw, ah->av.ib.dgid,16 ); }return 0 ;
@@ -100,3 +194,4 @@ int mlx4_ib_destroy_ah(struct ib_ah *ah)kfree (to_mah (ah));return 0 ; } +
drivers/infiniband/hw/mlx4/mad.c +20 -12
@@ -311,19 +311,25 @@ int mlx4_ib_mad_init(struct mlx4_ib_dev *dev)struct ib_mad_agent *agent;int p, q;int ret; +enum rdma_link_layer ll; -for (p =0 ; p < dev->num_ports; ++p) +for (p =0 ; p < dev->num_ports; ++p) { + ll =rdma_port_get_link_layer (&dev->ib_dev, p +1 );for (q =0 ; q <=1 ; ++q) { - agent =ib_register_mad_agent (&dev->ib_dev, p +1 , - q ?IB_QPT_GSI :IB_QPT_SMI , -NULL ,0 , send_handler, -NULL ,NULL ); -if (IS_ERR (agent)) { - ret =PTR_ERR (agent); - goto err; - } - dev->send_agent[p][q] = agent; +if (ll ==IB_LINK_LAYER_INFINIBAND ) { + agent =ib_register_mad_agent (&dev->ib_dev, p +1 , + q ?IB_QPT_GSI :IB_QPT_SMI , +NULL ,0 , send_handler, +NULL ,NULL ); +if (IS_ERR (agent)) { + ret =PTR_ERR (agent); + goto err; + } + dev->send_agent[p][q] = agent; + }else + dev->send_agent[p][q] =NULL ; } + }return 0 ;
@@ -344,8 +350,10 @@ void mlx4_ib_mad_cleanup(struct mlx4_ib_dev *dev)for (p =0 ; p < dev->num_ports; ++p) {for (q =0 ; q <=1 ; ++q) { agent = dev->send_agent[p][q]; - dev->send_agent[p][q] =NULL ; -ib_unregister_mad_agent (agent); +if (agent) { + dev->send_agent[p][q] =NULL ; +ib_unregister_mad_agent (agent); + } }if (dev->sm_ah[p])
drivers/infiniband/hw/mlx4/main.c +440 -34
611 lines not on this page · raw message
drivers/infiniband/hw/mlx4/mlx4_ib.h +30 -2
@@ -112,6 +112,13 @@ enum mlx4_ib_qp_flags { MLX4_IB_QP_BLOCK_MULTICAST_LOOPBACK = 1 << 1, }; +struct gid_entry { +struct list_head list; +union ib_gid gid; +int added; +u8 port; +}; +struct mlx4_ib_qp {struct ib_qp ibqp;struct mlx4_qp mqp;
@@ -138,6 +145,8 @@ struct mlx4_ib_qp {u8 resp_depth;u8 sq_no_prefetch;u8 state; +int mlx_type; +struct list_head gid_list; };struct mlx4_ib_srq {
@@ -157,7 +166,14 @@ struct mlx4_ib_srq {struct mlx4_ib_ah {struct ib_ah ibah; -struct mlx4_av av; +union mlx4_ext_av av; +}; + +struct mlx4_ib_iboe { +spinlock_t lock; +struct net_device *netdevs[MLX4_MAX_PORTS ]; +struct notifier_block nb; +union ib_gid gid_table[MLX4_MAX_PORTS ][128 ]; };struct mlx4_ib_dev {
@@ -176,6 +192,7 @@ struct mlx4_ib_dev {struct mutex cap_mask_mutex;bool ib_active; +struct mlx4_ib_iboe iboe; };static inline struct mlx4_ib_dev *to_mdev (struct ib_device *ibdev)
@@ -314,9 +331,20 @@ int mlx4_ib_map_phys_fmr(struct ib_fmr *ibfmr, u64 *page_list, int npages,int mlx4_ib_unmap_fmr (struct list_head *fmr_list);int mlx4_ib_fmr_dealloc (struct ib_fmr *fmr); +int mlx4_ib_resolve_grh (struct mlx4_ib_dev *dev,const struct ib_ah_attr *ah_attr, +u8 *mac,int *is_mcast,u8 port); +static inline int mlx4_ib_ah_grh_present (struct mlx4_ib_ah *ah) { -return !!(ah->av.g_slid &0x80 ); +u8 port =be32_to_cpu (ah->av.ib.port_pd) >>24 &3 ; + +if (rdma_port_get_link_layer (ah->ibah.device, port) ==IB_LINK_LAYER_ETHERNET ) +return 1 ; + +return !!(ah->av.ib.g_slid &0x80 ); } +int mlx4_ib_add_mc (struct mlx4_ib_dev *mdev,struct mlx4_ib_qp *mqp, +union ib_gid *gid); +#endif /* MLX4_IB_H */
drivers/infiniband/hw/mlx4/qp.c +112 -28
255 lines not on this page · raw message
drivers/net/mlx4/en_port.c +2 -2
@@ -127,8 +127,8 @@ int mlx4_SET_PORT_qpn_calc(struct mlx4_dev *dev, u8 port, u32 base_qpn,memset (context,0 ,sizeof *context); context->base_qpn =cpu_to_be32 (base_qpn); - context->promisc =cpu_to_be32 (promisc <<SET_PORT_PROMISC_SHIFT | base_qpn); - context->mcast =cpu_to_be32 (1 <<SET_PORT_PROMISC_SHIFT | base_qpn); + context->promisc =cpu_to_be32 (promisc <<SET_PORT_PROMISC_EN_SHIFT | base_qpn); + context->mcast =cpu_to_be32 (1 <<SET_PORT_PROMISC_MODE_SHIFT | base_qpn); context->intra_no_vlan =0 ; context->no_vlan =MLX4_NO_VLAN_IDX ; context->intra_vlan_miss =0 ;
drivers/net/mlx4/en_port.h +2 -1
@@ -36,7 +36,8 @@#define SET_PORT_GEN_ALL_VALID 0x7 -#define SET_PORT_PROMISC_SHIFT 31 +#define SET_PORT_PROMISC_EN_SHIFT 31 +#define SET_PORT_PROMISC_MODE_SHIFT 30enum {MLX4_CMD_SET_VLAN_FLTR =0x47 ,
drivers/net/mlx4/fw.c +2 -1
@@ -98,7 +98,8 @@ static void dump_dev_cap_flags(struct mlx4_dev *dev, u32 flags) [20] = "Address vector port checking support", [21] = "UD multicast support", [24] = "Demand paging support", - [25] = "Router support" + [25] = "Router support", + [30] = "IBoE support" };int i;
include/linux/mlx4/cmd.h +1 -0
@@ -140,6 +140,7 @@ enum {MLX4_SET_PORT_MAC_TABLE =0x2 ,MLX4_SET_PORT_VLAN_TABLE =0x3 ,MLX4_SET_PORT_PRIO_MAP =0x4 , + MLX4_SET_PORT_GID_TABLE = 0x5, };struct mlx4_dev ;
include/linux/mlx4/device.h +29 -1
@@ -67,7 +67,8 @@ enum {MLX4_DEV_CAP_FLAG_ATOMIC =1 <<18 ,MLX4_DEV_CAP_FLAG_RAW_MCAST =1 <<19 ,MLX4_DEV_CAP_FLAG_UD_AV_PORT =1 <<20 , -MLX4_DEV_CAP_FLAG_UD_MCAST =1 <<21 +MLX4_DEV_CAP_FLAG_UD_MCAST =1 <<21 , +MLX4_DEV_CAP_FLAG_IBOE =1 <<30 }; enum {
@@ -373,6 +374,27 @@ struct mlx4_av {u8 dgid[16 ]; }; +struct mlx4_eth_av { +__be32 port_pd; +u8 reserved1; +u8 smac_idx; +u16 reserved2; +u8 reserved3; +u8 gid_index; +u8 stat_rate; +u8 hop_limit; +__be32 sl_tclass_flowlabel; +u8 dgid[16 ]; +u32 reserved4[2 ]; +__be16 vlan; +u8 mac[6 ]; +}; + +union mlx4_ext_av { +struct mlx4_av ib; +struct mlx4_eth_av eth; +}; +struct mlx4_dev {struct pci_dev *pdev;unsigned long flags;
@@ -401,6 +423,12 @@ struct mlx4_init_port_param {if (((type) ==MLX4_PORT_TYPE_IB ? (dev)->caps.port_mask : \ ~(dev)->caps.port_mask) & 1 << ((port) - 1)) +#define mlx4_foreach_ib_transport_port (port, dev) \ + for ((port) = 1; (port) <= (dev)->caps.num_ports; (port)++) \ + if (((dev)->caps.port_mask & 1 << ((port) - 1)) || \ + ((dev)->caps.flags & MLX4_DEV_CAP_FLAG_IBOE)) + +int mlx4_buf_alloc (struct mlx4_dev *dev,int size,int max_direct,struct mlx4_buf *buf);void mlx4_buf_free (struct mlx4_dev *dev,int size,struct mlx4_buf *buf);
include/linux/mlx4/qp.h +5 -2
@@ -112,7 +112,8 @@ struct mlx4_qp_path {u8 snooper_flags;u8 reserved3[2 ];u8 counter_index; -u8 reserved4[7 ]; +u8 reserved4; +u8 dmac[6 ]; };struct mlx4_qp_context {
@@ -166,6 +167,7 @@ enum {MLX4_WQE_CTRL_TCP_UDP_CSUM =1 <<5 ,MLX4_WQE_CTRL_INS_VLAN =1 <<6 ,MLX4_WQE_CTRL_STRONG_ORDER =1 <<7 , + MLX4_WQE_CTRL_FORCE_LOOPBACK = 1 << 0, }; struct mlx4_wqe_ctrl_seg {
@@ -219,7 +221,8 @@ struct mlx4_wqe_datagram_seg {__be32 av[8 ];__be32 dqpn;__be32 qkey; -__be32 reservd[2 ]; +__be16 vlan; +u8 mac[6 ]; };struct mlx4_wqe_lso_seg {
Signature · 6 lines
1.7.2.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Just curious -- what's up with this change here? Is this connected to IBoE support, or is this an independent fix?
Quoted 20 lines
drivers/net/mlx4/en_port.c +2
-2@@ -127,8 +127,8 @@ int mlx4_SET_PORT_qpn_calc(struct mlx4_dev *dev, u8 port, u32 base_qpn,memset (context,0 ,sizeof *context); context->base_qpn =cpu_to_be32 (base_qpn); - context->promisc =cpu_to_be32 (promisc <<SET_PORT_PROMISC_SHIFT | base_qpn); - context->mcast =cpu_to_be32 (1 <<SET_PORT_PROMISC_SHIFT | base_qpn); + context->promisc =cpu_to_be32 (promisc <<SET_PORT_PROMISC_EN_SHIFT | base_qpn); + context->mcast =cpu_to_be32 (1 <<SET_PORT_PROMISC_MODE_SHIFT | base_qpn); context->intra_no_vlan =0 ; context->no_vlan =MLX4_NO_VLAN_IDX ; context->intra_vlan_miss =0 ;drivers/net/mlx4/en_port.h +0
-0
drivers/net/mlx4/en_port.h line 43
Quoted 9 lines
drivers/net/mlx4/en_port.h +2
-1@@ -36,7 +36,8 @@#define SET_PORT_GEN_ALL_VALID 0x7 -#define SET_PORT_PROMISC_SHIFT 31 +#define SET_PORT_PROMISC_EN_SHIFT 31 +#define SET_PORT_PROMISC_MODE_SHIFT 30enum {MLX4_CMD_SET_VLAN_FLTR =0x47 ,
Also as far as I can tell this variable sgid is write-only (ie you do the ib_get_cached_gid() but then never do anything with the value that that returns). Am I missing something subtle?
Quoted 9 lines
drivers/net/mlx4/en_port.h +4
-0@@ -1226,43 +1281,59 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,int header_size;int spc;int i; +union ib_gid sgid; +int is_eth; +int is_grh; +int err;
drivers/net/mlx4/en_port.h static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr, · line 1297
Quoted 10 lines
drivers/net/mlx4/en_port.h +4
-1send_size =0 ;for (i =0 ; i < wr->num_sge; ++i) send_size += wr->sg_list[i].length; -ib_ud_header_init (send_size,1 ,0 ,mlx4_ib_ah_grh_present (ah),0 , &sqp->ud_header); + is_eth =rdma_port_get_link_layer (sqp->qp.ibqp.device, sqp->qp.port) ==IB_LINK_LAYER_ETHERNET ; + is_grh =mlx4_ib_ah_grh_present (ah); + err =ib_get_cached_gid (ib_dev,be32_to_cpu (ah->av.ib.port_pd) >>24 , + ah->av.ib.gid_index, &sgid);
Finally this patch would have been easier to review without extraneous whitespace noise like
Quoted 10 lines
drivers/net/mlx4/en_port.h +1
-1@@ -1337,7 +1419,7 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr, } *mlx_seg_len = -ALIGN (i *sizeof (struct mlx4_wqe_inline_seg ) + header_size,16 ); +ALIGN (i *sizeof (struct mlx4_wqe_inline_seg ) + header_size,16 );return 0 ; }
drivers/net/mlx4/en_port.h int mlx4_ib_destroy_ah(struct ib_ah *ah) · line 197
Quoted 4 lines
drivers/net/mlx4/en_port.h +1
-0@@ -100,3 +194,4 @@ int mlx4_ib_destroy_ah(struct ib_ah *ah)kfree (to_mah (ah));return 0 ; } +
etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 2 lines · Roland Dreier
Just curious -- what's up with this change here? Is this connected to IBoE support, or is this an independent fix?
It is required for proper distribution of IBoE multicast packets while still not hurting the mlx4_en driver's operation.
Quoted 4 lines
Also as far as I can tell this variable sgid is write-only (ie you do the
ib_get_cached_gid()but then never do anything with the value that that returns). Am I missing something subtle?
It is not required in the context of this patch. It should have been part of patch 12/12 which adds VLAN support, in which case it is used to extract the required VLAN ID.
Roland, would you like me to send patches 8-12 again with this fixed?
Quoted 4 lines
Finally this patch would have been easier to review without extraneous whitespace noise like
Sorry about that. I pass them through checkpatch but this must have been hiden within the "line over ..." messages.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines
Quoted 2 lines
Just curious -- what's up with this change here? Is this connected to IBoE support, or is this an independent fix?
It is required for proper distribution of IBoE multicast packets while still not hurting the
mlx4_endriver's operation.
OK, I split this into a separate patch for clarity.
Quoted 5 lines
Quoted 2 lines
Finally this patch would have been easier to review without extraneous whitespace noise like
Sorry about that. I pass them through checkpatch but this must have been hiden within the "line over ..." messages.
It's not a checkpatch issue. It's just that you made random formatting changes to parts of the code that weren't otherwise touched.
Quoted 1 line
Roland, would you like me to send patches 8-12 again with this fixed?
I think I can still handle it.
However I notice one other thing that looks like a bug.
You add a function that starts:
int mlx4_ib_resolve_grh(struct mlx4_ib_dev *dev, const struct ib_ah_attr *ah_attr,
u8 *mac, int *is_mcast, u8 port)
{
struct mlx4_ib_iboe *iboe = &dev->iboe;
struct in6_addr in6;
*is_mcast = 0;
spin_lock(&iboe->lock);which is called from create_iboe_ah() which is called from mlx4_ib_create_ah(), which can be called from both interrupt and process context. So as far as I can tell, this spin_lock() needs to actually do spin_lock_irqsave()?
- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Roland Dreier
int mlx4_ib_resolve_grh(struct
mlx4_ib_dev*dev, const structib_ah_attr*ah_attr,
u8 *mac, int*is_mcast, u8 port){ struct mlx4_ib_iboe *iboe = &dev->iboe; struct in6_addr in6; *is_mcast = 0; spin_lock(&iboe->lock);
Quoted 5 lines
which is called from
create_iboe_ah()which is called frommlx4_ib_create_ah(), which can be called from both interrupt and process context. So as far as I can tell, thisspin_lock()needs to actually dospin_lock_irqsave()?
This was added in the past when I wanted to use the pointer to the net device to resolve any IPv6 address. Since we only resolve link local addresses, why don't we just remove the spinlocks and the check inside it?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines
This was added in the past when I wanted to use the pointer to the net device to resolve any IPv6 address. Since we only resolve link local addresses, why don't we just remove the spinlocks and the check inside it?
OK, easy enough.
- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Also I don't see anywhere that the global functions
drivers/infiniband/hw/mlx4/main.c static int mlx4_ib_query_port(struct ib_device *ibdev, u8 port, · line 220
Quoted 2 lines
drivers/infiniband/hw/mlx4/main.c +2
-0+int eth_to_ib_width(int w) +int eth_to_ib_speed(int s)
are called?
- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Roland Dreier
Also I don't see anywhere that the global functions
drivers/infiniband/hw/mlx4/main.c static int mlx4_ib_query_port(struct ib_device *ibdev, u8 port, · line 220
Quoted 2 lines
drivers/infiniband/hw/mlx4/main.c +2
-0+int eth_to_ib_width(int w) +int eth_to_ib_speed(int s)are called?
Right. Let's remove them.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
drivers/infiniband/hw/mlx4/qp.c enum { · line 62
Quoted 7 lines
enum {
drivers/infiniband/hw/mlx4/qp.c +2
-1/* * Largest possible UD header: send with GRH and immediate data. +* 4 bytes added to accommodate for eth header instead of lrh */ - MLX4_IB_UD_HEADER_SIZE = 72, + MLX4_IB_UD_HEADER_SIZE = 76,
I don't understand this change either. As far as I can tell, a 14-byte Ethernet header is 6 bytes longer than an 8-byte LRH, and with .1q the 18-byte header will require 10 bytes more than the LRH. So shouldn't this value be 78 in this patch and updated to 82 in the VLAN patch?
(It probably works in practice because the allocation of sqp structs gets rounded up enough)
- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 2 lines
(It probably works in practice because the allocation of sqp structs gets rounded up enough)
And I guess also no one actually ends up sending MADs with immediate data, so that saves 4 bytes off the worst case too...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines · Roland Dreier
Quoted 2 lines
(It probably works in practice because the allocation of sqp structs gets rounded up enough)
And I guess also no one actually ends up sending MADs with immediate data, so that saves 4 bytes off the worst case too...
I don't get this. For IB, we have 72 without immediate - did you forget to add the ICRC? I think we count it with the header... --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines
I don't get this. For IB, we have 72 without immediate - did you forget to add the ICRC? I think we count it with the header...
We don't need ICRC in the header_buf, do we? We're talking about MLX4_IB_UD_HEADER_SIZE, which is used to hold the packet header we get from ib_ud_header_pack(). The ICRC is accounted for in a different segment of the WQE.
In any case, it shouldn't matter -- I don't see how it could be correct to increase the buffer size by 4 bytes to account for the change from LRH to Ethernet header; an LRH is 8 bytes and an Ethernet header (without .1q etc) is 14 bytes. So we should add 6 bytes, right?
The math I get is
IB: LRH + GRH + BTH + DETH + IMM
8 40 12 8 4 = 72
Eth: MAC + GRH + BTH + DETH + IMM
14 40 12 8 4 = 78
I don't see how your change
drivers/infiniband/hw/mlx4/qp.c enum { · line 62
Quoted 2 lines
drivers/infiniband/hw/mlx4/qp.c +1
-1- MLX4_IB_UD_HEADER_SIZE = 72, + MLX4_IB_UD_HEADER_SIZE = 76,
could possibly be correct, although as I said it probably works because of padding and not hitting the worst case in practice anyway.
- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 18 lines · Roland Dreier
We don't need ICRC in the
header_buf, do we? We're talking aboutMLX4_IB_UD_HEADER_SIZE, which is used to hold the packet header we get fromib_ud_header_pack(). The ICRC is accounted for in a different segment of the WQE.In any case, it shouldn't matter -- I don't see how it could be correct to increase the buffer size by 4 bytes to account for the change from LRH to Ethernet header; an LRH is 8 bytes and an Ethernet header (without .1q etc) is 14 bytes. So we should add 6 bytes, right?
The math I get is
IB: LRH + GRH + BTH + DETH + IMM
8 40 12 8 4 = 72Eth: MAC + GRH + BTH + DETH + IMM
14 40 12 8 4 = 78
Quoted 8 lines
I don't see how your change
Review
Quoted 2 lines
(quoted diff) +1
-1- MLX4_IB_UD_HEADER_SIZE = 72, + MLX4_IB_UD_HEADER_SIZE = 76,could possibly be correct, although as I said it probably works because of padding and not hitting the worst case in practice anyway.
Sure, all clear.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Roland Dreier
drivers/infiniband/hw/mlx4/qp.c enum { · line 62
Quoted 7 lines
enum {
drivers/infiniband/hw/mlx4/qp.c +2
-1/* * Largest possible UD header: send with GRH and immediate data. +* 4 bytes added to accommodate for eth header instead of lrh */ - MLX4_IB_UD_HEADER_SIZE = 72, + MLX4_IB_UD_HEADER_SIZE = 76,I don't understand this change either. As far as I can tell, a 14-byte Ethernet header is 6 bytes longer than an 8-byte LRH, and with .1q the 18-byte header will require 10 bytes more than the LRH. So shouldn't this value be 78 in this patch and updated to 82 in the VLAN patch?
Quoted 3 lines
(It probably works in practice because the allocation of sqp structs gets rounded up enough)
Agree, I made a wrong calculation.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · Eli Cohen
[...] Address resolution is done atomically in the
case of a link local address or a multicast GID and otherwise-EINVALis returned. mlx4 transport packets were changed too to accommodate for IBoE. Multicast groups attach/detach calls dev_mc_add/remove to update the NIC's multicast filters.
This change log and also I assume the patch as well, deals alot with multicast, however, patch 0/10 says "With these patches, IBoE multicast frames may be broadcast as there is
currently no use of a L2 multicast group membership protocol." - does this means some/much of the code added/changed by this patch is dead code or not needed at this point?
Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html