diff mbox series

[v3,17/22] net/ena: support SMP for mz alloc counter

Message ID 20210506142526.28245-18-mk@semihalf.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series net/ena: update ENA PMD to v2.3.0 |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Michal Krawczyk May 6, 2021, 2:25 p.m. UTC 
  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.

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(-)

Comments

Ferruh Yigit May 7, 2021, 4:48 p.m. UTC | #1 
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.

Next patch sharing RSS key using this shared area, can you device private data
so all devices can access it.

> 
> 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 */
>
Stanislaw Kardach May 7, 2021, 5:18 p.m. UTC | #2 
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.
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 */
> > 
>
Ferruh Yigit May 9, 2021, 1:41 p.m. UTC | #3 
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.


Btw, How this shared memory freed?

> 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 */
>>>
>>
>
Stanislaw Kardach May 10, 2021, 8:18 a.m. UTC | #4 
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 */
> >>>
> >>
> > 
>
diff mbox series

Patch

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 */