Re: [dpdk-dev] [PATCH v3 17/22] net/ena: support SMP for mz alloc counter

[Stanislaw Kardach <kda@semihalf.com>]

On Sun, May 09, 2021 at 02:41:18PM +0100, Ferruh Yigit wrote:
> On 5/7/2021 6:18 PM, Stanislaw Kardach wrote:
> > On Fri, May 07, 2021 at 05:48:50PM +0100, Ferruh Yigit wrote:
> >> On 5/6/2021 3:25 PM, Michal Krawczyk wrote:
> >>> From: Stanislaw Kardach <kda@semihalf.com>
> >>>
> >>> Introduce a memory area for ENA driver shared between all the processes
> >>> of a same prefix (memzone backed).
> >>> Move the memzone allocation counter for ENA_MEM_ALLOC_COHERENT there so
> >>> that all processes may utilize it.
> >>
> >> Device private data is already shared between primary/secondary processes, why
> >> not using it, it is already there.
> > Please correct me if I'm wrong, the dev->data->dev_private is a
> > per-device space, whereas the memzone here is used as a shared memory
> > between all ena devices.
> 
> Yes it is per-device, so instead of keeping the shared are pointer as global
> variable, it is possible to keep this pointer in the device private area, and
> initialize per device. This way shared area can be reached by both primary and
> secondary applications without additional check.
> I think this works better to store device related shared data, like RSS key.
> 
> But I am not sure about 'ena_alloc_cnt', I am not clear what it is, it looks
> like intention is to make it accesible from all devices and all processes.
Come to think of it, I think we can completely eliminate this memzone.
ena_alloc_cnt is used for unique memzone name generation but same could
be achieved with port_id + per_device_counter.
Thanks for the suggestion, we'll re-think this patch.
> 
> 
> Btw, How this shared memory freed?
Since I think we can get by without it, this question becomes academic,
though interesting.
Short answer is can't be freed. The reason for that is in multi-process
mode the primary process may die while secondaries continue. It means the
daemon thread which handles FD passing will die and hence secondaries
cannot perform memory alloc/free.
So even using reference count to delay memzone free won't help us if the
primary is dead.
> 
> > More precisely the memzone counter used here is required to wrap some of
> > the base ena code we are calling and may be called from the context of
> > any device. Given that memzone names have to be unique, I need this
> > counter to be unique too.
> > I believe similar thing is done in mlx5 driver
> > (mlx5_init_shared_data()). If there is a better way to register such a
> > shared segment that is going to be preserved until all processes
> > (primary and secondary) are closed I would be happy to rework this.
> >>
> >> Next patch sharing RSS key using this shared area, can you device private data
> >> so all devices can access it.
> > It is somewhat similar case. The default key there is generated once for
> > all devices and then used in each of them.
> >>
> >>>
> >>> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> >>> Reviewed-by: Michal Krawczyk <mk@semihalf.com>
> >>> Reviewed-by: Igor Chauskin <igorch@amazon.com>
> >>> Reviewed-by: Shay Agroskin <shayagr@amazon.com>
> >>> ---
> >>>  drivers/net/ena/base/ena_plat_dpdk.h |  6 ++--
> >>>  drivers/net/ena/ena_ethdev.c         | 46 +++++++++++++++++++++++++++-
> >>>  drivers/net/ena/ena_ethdev.h         |  8 +++++
> >>>  3 files changed, 56 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ena/base/ena_plat_dpdk.h b/drivers/net/ena/base/ena_plat_dpdk.h
> >>> index 1d0454bebe..e17970361a 100644
> >>> --- a/drivers/net/ena/base/ena_plat_dpdk.h
> >>> +++ b/drivers/net/ena/base/ena_plat_dpdk.h
> >>> @@ -209,7 +209,7 @@ typedef struct {
> >>>   * Each rte_memzone should have unique name.
> >>>   * To satisfy it, count number of allocations and add it to name.
> >>>   */
> >>> -extern rte_atomic64_t ena_alloc_cnt;
> >>> +extern rte_atomic64_t *ena_alloc_cnt;
> >>>  
> >>>  #define ENA_MEM_ALLOC_COHERENT_ALIGNED(					       \
> >>>  	dmadev, size, virt, phys, mem_handle, alignment)		       \
> >>> @@ -219,7 +219,7 @@ extern rte_atomic64_t ena_alloc_cnt;
> >>>  		if (size > 0) {						       \
> >>>  			char z_name[RTE_MEMZONE_NAMESIZE];		       \
> >>>  			snprintf(z_name, sizeof(z_name), "ena_alloc_%"PRIi64"",\
> >>> -				rte_atomic64_add_return(&ena_alloc_cnt,	1));   \
> >>> +				rte_atomic64_add_return(ena_alloc_cnt, 1));    \
> >>>  			mz = rte_memzone_reserve_aligned(z_name, size,	       \
> >>>  					SOCKET_ID_ANY, RTE_MEMZONE_IOVA_CONTIG,\
> >>>  					alignment);			       \
> >>> @@ -249,7 +249,7 @@ extern rte_atomic64_t ena_alloc_cnt;
> >>>  		if (size > 0) {						       \
> >>>  			char z_name[RTE_MEMZONE_NAMESIZE];		       \
> >>>  			snprintf(z_name, sizeof(z_name), "ena_alloc_%"PRIi64"",\
> >>> -				rte_atomic64_add_return(&ena_alloc_cnt, 1));   \
> >>> +				rte_atomic64_add_return(ena_alloc_cnt, 1));    \
> >>>  			mz = rte_memzone_reserve_aligned(z_name, size,	       \
> >>>  				node, RTE_MEMZONE_IOVA_CONTIG, alignment);     \
> >>>  			mem_handle = mz;				       \
> >>> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> >>> index 5d107775f4..0780e2fee2 100644
> >>> --- a/drivers/net/ena/ena_ethdev.c
> >>> +++ b/drivers/net/ena/ena_ethdev.c
> >>> @@ -83,11 +83,15 @@ struct ena_stats {
> >>>  /* Device arguments */
> >>>  #define ENA_DEVARG_LARGE_LLQ_HDR "large_llq_hdr"
> >>>  
> >>> +#define ENA_MZ_SHARED_DATA "ena_shared_data"
> >>> +
> >>>  /*
> >>>   * Each rte_memzone should have unique name.
> >>>   * To satisfy it, count number of allocation and add it to name.
> >>>   */
> >>> -rte_atomic64_t ena_alloc_cnt;
> >>> +rte_atomic64_t *ena_alloc_cnt;
> >>> +
> >>> +struct ena_shared_data *ena_shared_data;
> >>>  
> >>>  static const struct ena_stats ena_stats_global_strings[] = {
> >>>  	ENA_STAT_GLOBAL_ENTRY(wd_expired),
> >>> @@ -1752,6 +1756,42 @@ static uint32_t ena_calc_max_io_queue_num(struct ena_com_dev *ena_dev,
> >>>  	return max_num_io_queues;
> >>>  }
> >>>  
> >>> +static void ena_prepare_shared_data(struct ena_shared_data *shared_data)
> >>> +{
> >>> +	memset(shared_data, 0, sizeof(*shared_data));
> >>> +}
> >>> +
> >>> +static int ena_shared_data_init(void)
> >>> +{
> >>> +	const struct rte_memzone *mz;
> >>> +
> >>> +	if (ena_shared_data != NULL)
> >>> +		return 0;
> >>> +
> >>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> >>> +		/* Allocate shared memory. */
> >>> +		mz = rte_memzone_reserve(ENA_MZ_SHARED_DATA,
> >>> +					 sizeof(*ena_shared_data),
> >>> +					 SOCKET_ID_ANY, 0);
> >>> +		if (mz == NULL) {
> >>> +			PMD_INIT_LOG(CRIT, "Cannot allocate ena shared data");
> >>> +			return -rte_errno;
> >>> +		}
> >>> +		ena_prepare_shared_data(mz->addr);
> >>> +	} else {
> >>> +		/* Lookup allocated shared memory. */
> >>> +		mz = rte_memzone_lookup(ENA_MZ_SHARED_DATA);
> >>> +		if (mz == NULL) {
> >>> +			PMD_INIT_LOG(CRIT, "Cannot attach ena shared data");
> >>> +			return -rte_errno;
> >>> +		}
> >>> +	}
> >>> +	ena_shared_data = mz->addr;
> >>> +	/* Setup ENA_MEM memzone name counter. */
> >>> +	ena_alloc_cnt = &ena_shared_data->mz_alloc_cnt;
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
> >>>  {
> >>>  	struct ena_calc_queue_size_ctx calc_queue_ctx = { 0 };
> >>> @@ -1773,6 +1813,10 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
> >>>  	eth_dev->tx_pkt_burst = &eth_ena_xmit_pkts;
> >>>  	eth_dev->tx_pkt_prepare = &eth_ena_prep_pkts;
> >>>  
> >>> +	rc = ena_shared_data_init();
> >>> +	if (rc != 0)
> >>> +		return rc;
> >>> +
> >>>  	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>>  		return 0;
> >>>  
> >>> diff --git a/drivers/net/ena/ena_ethdev.h b/drivers/net/ena/ena_ethdev.h
> >>> index ae235897ee..e8858c6118 100644
> >>> --- a/drivers/net/ena/ena_ethdev.h
> >>> +++ b/drivers/net/ena/ena_ethdev.h
> >>> @@ -207,6 +207,14 @@ struct ena_offloads {
> >>>  	bool rx_csum_supported;
> >>>  };
> >>>  
> >>> +/* Holds data shared between all instances of ENA PMD. */
> >>> +struct ena_shared_data {
> >>> +	/* Each rte_memzone should have unique name.
> >>> +	 * To satisfy it, count number of allocation and add it to name.
> >>> +	 */
> >>> +	rte_atomic64_t mz_alloc_cnt;
> >>> +};
> >>> +
> >>>  /* board specific private data structure */
> >>>  struct ena_adapter {
> >>>  	/* OS defined structs */
> >>>
> >>
> > 
> 

-- 
Best Regards,
Stanislaw Kardach