Re: [PATCH 06/14] net/mlx5: add IB shared context alloc/free functions

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, March 21, 2019 10:11 AM, Viacheslav Ovsiienko:
> Subject: [dpdk-dev] [PATCH 06/14] net/mlx5: add IB shared context
> alloc/free functions
> 
> The functions to allocate and free shared IB context for multiport is added.
> The IB device context, Protection Domain, device attributes, Infiniband
> names are going to be relocated to the shared structure from the device
> private one. mlx5_dev_spawn() is updated to create shared context.
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

Patch #2 in the series should be part of this commit, since it doesn't make sense to be a standalone. 

> ---
>  drivers/net/mlx5/mlx5.c | 234
> ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 176 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> 100e9f4..b3060de 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -140,6 +140,150 @@ struct mlx5_dev_spawn_data {
>  	struct rte_eth_dev *eth_dev; /**< Associated Ethernet device. */  };
> 
> +static LIST_HEAD(, mlx5_ibv_shared) mlx5_ibv_list =
> +LIST_HEAD_INITIALIZER();
> +
> +/**
> + * Allocate shared IB device context. If there is multiport device the
> + * master and representors will share this context, if there is single
> + * port dedicated IB device, the context will be used by only given
> + * port due to unification.
> + *
> + * Routine first searches the context for the spesified IB device name,
> + * if found the shared context assumed and reference counter is
> incremented.
> + * If no context found the new one is created and initialized with
> +specified
> + * IB device context and parameters.
> + *
> + * @param[in] spawn
> + *   Pointer to the IB device attributes (name, port, etc).
> + *
> + * @return
> + *   Pointer to mlx5_ibv_shared object on success,
> + *   otherwise NULL and rte_errno is set.
> + */
> +static struct mlx5_ibv_shared *
> +mlx5_alloc_shared_ibctx(const struct mlx5_dev_spawn_data *spawn) {
> +	struct mlx5_ibv_shared *sh;
> +	int err = 0;

Since you having a global list you need to make the access thread safe. 

> +
> +	assert(spawn);
> +	/* Search for IB context by device name. */
> +	LIST_FOREACH(sh, &mlx5_ibv_list, next) {
> +		if (!strcmp(sh->ibdev_name, spawn->ibv_dev->name)) {
> +			assert(!sh->secondary);

How do you enforce secondary not to call this function? 

> +			sh->refcnt++;
> +			return sh;
> +		}
> +	}
> +	/* No device found, we have to create new sharted context. */
> +	assert(spawn->max_port);
> +	sh = rte_zmalloc("ethdev shared ib context",
> +			 sizeof(struct mlx5_ibv_shared) +
> +			 spawn->max_port *
> +			 sizeof(struct mlx5_ibv_shared_port),
> +			 RTE_CACHE_LINE_SIZE);
> +	if (!sh) {
> +		DRV_LOG(ERR, "shared context allocation failure");
> +		rte_errno  = ENOMEM;
> +		return NULL;
> +	}
> +	/* Try to open IB device with DV first, then usual Verbs. */
> +	errno = 0;
> +	sh->ctx = mlx5_glue->dv_open_device(spawn->ibv_dev);

This one is only for primary processes right? 

> +	if (sh->ctx) {
> +		sh->devx = 1;
> +		DRV_LOG(DEBUG, "DevX is supported");
> +	} else {
> +		sh->ctx = mlx5_glue->open_device(spawn->ibv_dev);
> +		if (!sh->ctx) {
> +			err = errno ? errno : ENODEV;
> +			goto error;
> +		}
> +		DRV_LOG(DEBUG, "DevX is NOT supported");
> +	}
> +	err = mlx5_glue->query_device_ex(sh->ctx, NULL, &sh-
> >device_attr);
> +	if (err) {
> +		DRV_LOG(DEBUG, "ibv_query_device_ex() failed");
> +		goto error;
> +	}
> +	sh->refcnt = 1;
> +	sh->max_port = spawn->max_port;
> +	strncpy(sh->ibdev_name, sh->ctx->device->name,
> +		sizeof(sh->ibdev_name));
> +	strncpy(sh->ibdev_path, sh->ctx->device->ibdev_path,
> +		sizeof(sh->ibdev_path));
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		/*
> +		 * For secondary process we just open the IB device
> +		 * and get attributes, there will no be real usage
> +		 * of this structure, the secondary process will
> +		 * use one from prpimary.
> +		 */
> +		sh->secondary = 1;

Secondary process should not open a new device. it should use the primary device + private structure for everything.
In fact, secondary process should not call this function nor reference any shared object. 

> +		return sh;
> +	}
> +	sh->pd = mlx5_glue->alloc_pd(sh->ctx);
> +	if (sh->pd == NULL) {
> +		DRV_LOG(ERR, "PD allocation failure");
> +		err = ENOMEM;
> +		goto error;
> +	}
> +	LIST_INSERT_HEAD(&mlx5_ibv_list, sh, next);
> +	return sh;
> +error:
> +	assert(sh);
> +	if (sh->pd)
> +		claim_zero(mlx5_glue->dealloc_pd(sh->pd));
> +	if (sh->ctx)
> +		claim_zero(mlx5_glue->close_device(sh->ctx));
> +	rte_free(sh);
> +	assert(err > 0);
> +	rte_errno = err;
> +	return NULL;
> +}
> +
> +/**
> + * Free shared IB device context. Decrement counter and if zero free
> + * all allocated resources and close handles.
> + *
> + * @param[in] sh
> + *   Pointer to mlx5_ibv_shared object to free
> + */
> +static void
> +mlx5_free_shared_ibctx(struct mlx5_ibv_shared *sh) { #ifndef NDEBUG
> +	/* Check the object presence in the list. */
> +	struct mlx5_ibv_shared *lctx;
> +
> +	LIST_FOREACH(lctx, &mlx5_ibv_list, next)
> +		if (lctx == sh)
> +			break;
> +	assert(lctx);
> +	if (lctx != sh) {
> +		DRV_LOG(ERR, "Freeing non-existing shared IB context");
> +		return;
> +	}
> +#endif
> +	assert(sh);
> +	assert(sh->refcnt);
> +	if (--sh->refcnt)
> +		return;
> +	/* Zero reference counter, we should release resources. */
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		assert(sh->secondary);
> +		assert(sh->ctx);
> +		assert(!sh->pd);
> +	}
> +	LIST_REMOVE(sh, next);

Secondary process is not allowed to do all of the below. 

> +	if (sh->pd)
> +		claim_zero(mlx5_glue->dealloc_pd(sh->pd));
> +	if (sh->ctx)
> +		claim_zero(mlx5_glue->close_device(sh->ctx));
> +	rte_free(sh);
> +}
> +
> +
>  /**
>   * Prepare shared data between primary and secondary process.
>   */
> @@ -289,12 +433,10 @@ struct mlx5_dev_spawn_data {
>  	}
>  	mlx5_mprq_free_mp(dev);
>  	mlx5_mr_release(dev);
> -	if (priv->pd != NULL) {
> -		assert(priv->ctx != NULL);
> -		claim_zero(mlx5_glue->dealloc_pd(priv->pd));
> -		claim_zero(mlx5_glue->close_device(priv->ctx));
> -	} else
> -		assert(priv->ctx == NULL);
> +	assert(priv->sh);
> +	if (priv->sh)
> +		mlx5_free_shared_ibctx(priv->sh);
> +	priv->sh = NULL;
>  	if (priv->rss_conf.rss_key != NULL)
>  		rte_free(priv->rss_conf.rss_key);
>  	if (priv->reta_idx != NULL)
> @@ -744,11 +886,8 @@ struct mlx5_dev_spawn_data {
>  	       struct mlx5_dev_config config)
>  {
>  	const struct mlx5_switch_info *switch_info = &spawn->info;
> -	struct ibv_device *ibv_dev = spawn->ibv_dev;
> -	struct ibv_context *ctx = NULL;
> -	struct ibv_device_attr_ex attr;
> +	struct mlx5_ibv_shared *sh;
>  	struct ibv_port_attr port_attr;
> -	struct ibv_pd *pd = NULL;
>  	struct mlx5dv_context dv_attr = { .comp_mask = 0 };
>  	struct rte_eth_dev *eth_dev = NULL;
>  	struct mlx5_priv *priv = NULL;
> @@ -807,18 +946,10 @@ struct mlx5_dev_spawn_data {
>  	}
>  	/* Prepare shared data between primary and secondary process. */
>  	mlx5_prepare_shared_data();
> -	errno = 0;
> -	ctx = mlx5_glue->dv_open_device(ibv_dev);
> -	if (ctx) {
> -		config.devx = 1;
> -		DRV_LOG(DEBUG, "DEVX is supported");
> -	} else {
> -		ctx = mlx5_glue->open_device(ibv_dev);
> -		if (!ctx) {
> -			rte_errno = errno ? errno : ENODEV;
> -			return NULL;
> -		}
> -	}
> +	sh = mlx5_alloc_shared_ibctx(spawn);
> +	if (!sh)
> +		return NULL;
> +	config.devx = sh->devx;
>  #ifdef HAVE_IBV_MLX5_MOD_SWP
>  	dv_attr.comp_mask |= MLX5DV_CONTEXT_MASK_SWP;  #endif @@
> -832,7 +963,7 @@ struct mlx5_dev_spawn_data {  #ifdef
> HAVE_IBV_DEVICE_STRIDING_RQ_SUPPORT
>  	dv_attr.comp_mask |= MLX5DV_CONTEXT_MASK_STRIDING_RQ;
> #endif
> -	mlx5_glue->dv_query_device(ctx, &dv_attr);
> +	mlx5_glue->dv_query_device(sh->ctx, &dv_attr);
>  	if (dv_attr.flags & MLX5DV_CONTEXT_FLAGS_MPW_ALLOWED) {
>  		if (dv_attr.flags &
> MLX5DV_CONTEXT_FLAGS_ENHANCED_MPW) {
>  			DRV_LOG(DEBUG, "enhanced MPW is supported");
> @@ -917,11 +1048,6 @@ struct mlx5_dev_spawn_data {
>  		" old OFED/rdma-core version or firmware configuration");
> #endif
>  	config.mpls_en = mpls_en;
> -	err = mlx5_glue->query_device_ex(ctx, NULL, &attr);
> -	if (err) {
> -		DEBUG("ibv_query_device_ex() failed");
> -		goto error;
> -	}
>  	DRV_LOG(DEBUG, "naming Ethernet device \"%s\"", name);
>  	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>  		eth_dev = rte_eth_dev_attach_secondary(name);
> @@ -957,11 +1083,11 @@ struct mlx5_dev_spawn_data {
>  		 */
>  		eth_dev->rx_pkt_burst =
> mlx5_select_rx_function(eth_dev);
>  		eth_dev->tx_pkt_burst =
> mlx5_select_tx_function(eth_dev);
> -		claim_zero(mlx5_glue->close_device(ctx));
> +		mlx5_free_shared_ibctx(sh);
>  		return eth_dev;
>  	}
>  	/* Check port status. */
> -	err = mlx5_glue->query_port(ctx, spawn->ibv_port, &port_attr);
> +	err = mlx5_glue->query_port(sh->ctx, spawn->ibv_port, &port_attr);
>  	if (err) {
>  		DRV_LOG(ERR, "port query failed: %s", strerror(err));
>  		goto error;
> @@ -975,13 +1101,7 @@ struct mlx5_dev_spawn_data {
>  		DRV_LOG(DEBUG, "port is not active: \"%s\" (%d)",
>  			mlx5_glue->port_state_str(port_attr.state),
>  			port_attr.state);
> -	/* Allocate protection domain. */
> -	pd = mlx5_glue->alloc_pd(ctx);
> -	if (pd == NULL) {
> -		DRV_LOG(ERR, "PD allocation failure");
> -		err = ENOMEM;
> -		goto error;
> -	}
> +	/* Allocate private eth device data. */
>  	priv = rte_zmalloc("ethdev private structure",
>  			   sizeof(*priv),
>  			   RTE_CACHE_LINE_SIZE);
> @@ -990,13 +1110,11 @@ struct mlx5_dev_spawn_data {
>  		err = ENOMEM;
>  		goto error;
>  	}
> -	priv->ctx = ctx;
> -	strncpy(priv->ibdev_name, priv->ctx->device->name,
> -		sizeof(priv->ibdev_name));
> -	strncpy(priv->ibdev_path, priv->ctx->device->ibdev_path,
> -		sizeof(priv->ibdev_path));
> -	priv->device_attr = attr;
> -	priv->pd = pd;
> +	priv->sh = sh;
> +	priv->ctx = sh->ctx;
> +	priv->ibv_port = spawn->ibv_port;
> +	priv->device_attr = sh->device_attr;
> +	priv->pd = sh->pd;
>  	priv->mtu = ETHER_MTU;
>  #ifndef RTE_ARCH_64
>  	/* Initialize UAR access locks for 32bit implementations. */ @@ -
> 1051,7 +1169,8 @@ struct mlx5_dev_spawn_data {
>  			strerror(rte_errno));
>  		goto error;
>  	}
> -	config.hw_csum = !!(attr.device_cap_flags_ex &
> IBV_DEVICE_RAW_IP_CSUM);
> +	config.hw_csum = !!(sh->device_attr.device_cap_flags_ex &
> +			    IBV_DEVICE_RAW_IP_CSUM);
>  	DRV_LOG(DEBUG, "checksum offloading is %ssupported",
>  		(config.hw_csum ? "" : "not "));
>  #if !defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) && \ @@ -1065,7
> +1184,7 @@ struct mlx5_dev_spawn_data {
>  	}
>  #endif
>  	config.ind_table_max_size =
> -		attr.rss_caps.max_rwq_indirection_table_size;
> +		sh->device_attr.rss_caps.max_rwq_indirection_table_size;
>  	/*
>  	 * Remove this check once DPDK supports larger/variable
>  	 * indirection tables.
> @@ -1074,18 +1193,18 @@ struct mlx5_dev_spawn_data {
>  		config.ind_table_max_size = ETH_RSS_RETA_SIZE_512;
>  	DRV_LOG(DEBUG, "maximum Rx indirection table size is %u",
>  		config.ind_table_max_size);
> -	config.hw_vlan_strip = !!(attr.raw_packet_caps &
> +	config.hw_vlan_strip = !!(sh->device_attr.raw_packet_caps &
> 
> IBV_RAW_PACKET_CAP_CVLAN_STRIPPING);
>  	DRV_LOG(DEBUG, "VLAN stripping is %ssupported",
>  		(config.hw_vlan_strip ? "" : "not "));
> -	config.hw_fcs_strip = !!(attr.raw_packet_caps &
> +	config.hw_fcs_strip = !!(sh->device_attr.raw_packet_caps &
>  				 IBV_RAW_PACKET_CAP_SCATTER_FCS);
>  	DRV_LOG(DEBUG, "FCS stripping configuration is %ssupported",
>  		(config.hw_fcs_strip ? "" : "not "));  #if
> defined(HAVE_IBV_WQ_FLAG_RX_END_PADDING)
> -	hw_padding = !!attr.rx_pad_end_addr_align;
> +	hw_padding = !!sh->device_attr.rx_pad_end_addr_align;
>  #elif defined(HAVE_IBV_WQ_FLAGS_PCI_WRITE_END_PADDING)
> -	hw_padding = !!(attr.device_cap_flags_ex &
> +	hw_padding = !!(sh->device_attr.device_cap_flags_ex &
>  			IBV_DEVICE_PCI_WRITE_END_PADDING);
>  #endif
>  	if (config.hw_padding && !hw_padding) { @@ -1094,11 +1213,11 @@
> struct mlx5_dev_spawn_data {
>  	} else if (config.hw_padding) {
>  		DRV_LOG(DEBUG, "Rx end alignment padding is enabled");
>  	}
> -	config.tso = (attr.tso_caps.max_tso > 0 &&
> -		      (attr.tso_caps.supported_qpts &
> +	config.tso = (sh->device_attr.tso_caps.max_tso > 0 &&
> +		      (sh->device_attr.tso_caps.supported_qpts &
>  		       (1 << IBV_QPT_RAW_PACKET)));
>  	if (config.tso)
> -		config.tso_max_payload_sz = attr.tso_caps.max_tso;
> +		config.tso_max_payload_sz = sh-
> >device_attr.tso_caps.max_tso;
>  	/*
>  	 * MPW is disabled by default, while the Enhanced MPW is enabled
>  	 * by default.
> @@ -1257,7 +1376,8 @@ struct mlx5_dev_spawn_data {
>  		.free = &mlx5_free_verbs_buf,
>  		.data = priv,
>  	};
> -	mlx5_glue->dv_set_context_attr(ctx,
> MLX5DV_CTX_ATTR_BUF_ALLOCATORS,
> +	mlx5_glue->dv_set_context_attr(sh->ctx,
> +				       MLX5DV_CTX_ATTR_BUF_ALLOCATORS,
>  				       (void *)((uintptr_t)&alctr));
>  	/* Bring Ethernet device up. */
>  	DRV_LOG(DEBUG, "port %u forcing Ethernet interface up", @@ -
> 1311,15 +1431,13 @@ struct mlx5_dev_spawn_data {
>  		if (eth_dev != NULL)
>  			eth_dev->data->dev_private = NULL;
>  	}
> -	if (pd)
> -		claim_zero(mlx5_glue->dealloc_pd(pd));
>  	if (eth_dev != NULL) {
>  		/* mac_addrs must not be freed alone because part of
> dev_private */
>  		eth_dev->data->mac_addrs = NULL;
>  		rte_eth_dev_release_port(eth_dev);
>  	}
> -	if (ctx)
> -		claim_zero(mlx5_glue->close_device(ctx));
> +	if (sh)
> +		mlx5_free_shared_ibctx(sh);
>  	assert(err > 0);
>  	rte_errno = err;
>  	return NULL;
> --
> 1.8.3.1