[dpdk-dev] [PATCH 0/3] net/enic: fix multi-process isseus

[dpdk-dev] [PATCH 0/3] net/enic: fix multi-process isseus

[Hyong Youb Kim]

This series fixes a few multi-process issues, mainly discovered by
Dirk-Holger Lenz. They manifest clearly when primary and secondary
processes use different executable images (hence different rte_eth_dev
addresses and so on).

Dirk has tested this series, and his app works fine. Below is the
original email thread.
http://mails.dpdk.org/archives/dev/2019-August/141560.html

This series replaces the earlier one.
http://patches.dpdk.org/patch/58033/

Thanks.
-Hyong

Hyong Youb Kim (3):
  net/enic: restrict several handlers to primary process
  net/enic: fix two issues in probe for secondary process
  net/enic: fix crash in secondary process

 drivers/net/enic/enic.h               |  8 +++--
 drivers/net/enic/enic_ethdev.c        | 27 ++++++++++++---
 drivers/net/enic/enic_main.c          | 50 +++++++++++++++++----------
 drivers/net/enic/enic_rxtx_vec_avx2.c |  5 ++-
 4 files changed, 62 insertions(+), 28 deletions(-)

-- 
2.22.0

[dpdk-dev] [PATCH 1/3] net/enic: restrict several handlers to primary process

[Hyong Youb Kim]

These eth_dev_ops handlers should run only in the primary process.
- filter_ctrl
- reta_update
- rss_hash_update
- set_mc_addr_list
- udp_tunnel_port_add
- udp_tunnel_port_del

Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
Fixes: 8d496995346c ("net/enic: support multicast filtering")
Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port number")
Cc: stable@dpdk.org

Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 06dc67122..85d785e62 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
 {
 	int ret = 0;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 
 	switch (filter_type) {
@@ -710,6 +712,8 @@ static int enicpmd_set_mc_addr_list(struct rte_eth_dev *eth_dev,
 	uint32_t i, j;
 	int ret;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 
 	/* Validate the given addresses first */
@@ -826,6 +830,8 @@ static int enicpmd_dev_rss_reta_update(struct rte_eth_dev *dev,
 	union vnic_rss_cpu rss_cpu;
 	uint16_t i, idx, shift;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	if (reta_size != ENIC_RSS_RETA_SIZE) {
 		dev_err(enic, "reta_update: wrong reta_size. given=%u"
@@ -854,6 +860,8 @@ static int enicpmd_dev_rss_hash_update(struct rte_eth_dev *dev,
 {
 	struct enic *enic = pmd_priv(dev);
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	return enic_set_rss_conf(enic, rss_conf);
 }
@@ -986,6 +994,8 @@ static int enicpmd_dev_udp_tunnel_port_add(struct rte_eth_dev *eth_dev,
 	struct enic *enic = pmd_priv(eth_dev);
 	int ret;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	ret = udp_tunnel_common_check(enic, tnl);
 	if (ret)
@@ -1008,6 +1018,8 @@ static int enicpmd_dev_udp_tunnel_port_del(struct rte_eth_dev *eth_dev,
 	struct enic *enic = pmd_priv(eth_dev);
 	int ret;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -E_RTE_SECONDARY;
 	ENICPMD_FUNC_TRACE();
 	ret = udp_tunnel_common_check(enic, tnl);
 	if (ret)
-- 
2.22.0

Re: [dpdk-dev] [PATCH 1/3] net/enic: restrict several handlers to primary process

[Ferruh Yigit]

On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
> These eth_dev_ops handlers should run only in the primary process.
> - filter_ctrl
> - reta_update
> - rss_hash_update
> - set_mc_addr_list
> - udp_tunnel_port_add
> - udp_tunnel_port_del
> 
> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
> Fixes: 8d496995346c ("net/enic: support multicast filtering")
> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port number")
> Cc: stable@dpdk.org
> 
> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> Reviewed-by: John Daley <johndale@cisco.com>
> ---
>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index 06dc67122..85d785e62 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
>  {
>  	int ret = 0;
>  
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -E_RTE_SECONDARY;
>  	ENICPMD_FUNC_TRACE();
>  
>  	switch (filter_type) {

I remember we have similar talk with John in the past about these secondary
application checks in ethdev_ops.

I would like to understand why these checks required only in enic, can you
please describe your use case?
Is there any reason secondary application can't change configuration of the
device, or are you updating your driver to work with specific application?

Re: [dpdk-dev] [PATCH 1/3] net/enic: restrict several handlers to primary process

[Hyong Youb Kim (hyonkim)]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 9, 2019 5:02 PM
> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
> process
> 
> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
> > These eth_dev_ops handlers should run only in the primary process.
> > - filter_ctrl
> > - reta_update
> > - rss_hash_update
> > - set_mc_addr_list
> > - udp_tunnel_port_add
> > - udp_tunnel_port_del
> >
> > Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
> > Fixes: 8d496995346c ("net/enic: support multicast filtering")
> > Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
> number")
> > Cc: stable@dpdk.org
> >
> > Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> > Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> > Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> > Reviewed-by: John Daley <johndale@cisco.com>
> > ---
> >  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/net/enic/enic_ethdev.c
> b/drivers/net/enic/enic_ethdev.c
> > index 06dc67122..85d785e62 100644
> > --- a/drivers/net/enic/enic_ethdev.c
> > +++ b/drivers/net/enic/enic_ethdev.c
> > @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
> >  {
> >  	int ret = 0;
> >
> > +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return -E_RTE_SECONDARY;
> >  	ENICPMD_FUNC_TRACE();
> >
> >  	switch (filter_type) {
> 
> I remember we have similar talk with John in the past about these secondary
> application checks in ethdev_ops.
> 
> I would like to understand why these checks required only in enic, can you
> please describe your use case?
> Is there any reason secondary application can't change configuration of the
> device, or are you updating your driver to work with specific application?

Hi,

No fundamental reasons why secondary processes cannot run these
handlers. These checks are to make it clear that it is not safe to do
so at the moment. It is a software limitation.

The firmware API (devcmd) we use to configure NIC settings assumes one
user executing one command at a time. And, many of the handlers in the
driver also assume primary process. The firmware itself has
enough checks to prevent concurrent devcmd attempts from corrupting
its internal state. But, host processes can get confused. For example,
process A gets process B's results, or overwrites B's devcmd, etc.

I believe these issues are all fixable in the driver. We could use
locks in shared memory to serialize devcmd (though risky), fix
handlers that assume primary process, and so on. It is a to-do item for
this driver and would require its own patch series (e.g. allow
secondary processes to run X, Y, Z safely)..

Thanks.
-Hyong

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to primary process

[Ferruh Yigit]

On 10/9/2019 9:48 AM, Hyong Youb Kim (hyonkim) wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, October 9, 2019 5:02 PM
>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
>> process
>>
>> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
>>> These eth_dev_ops handlers should run only in the primary process.
>>> - filter_ctrl
>>> - reta_update
>>> - rss_hash_update
>>> - set_mc_addr_list
>>> - udp_tunnel_port_add
>>> - udp_tunnel_port_del
>>>
>>> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
>>> Fixes: 8d496995346c ("net/enic: support multicast filtering")
>>> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
>> number")
>>> Cc: stable@dpdk.org
>>>
>>> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
>>> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>> Reviewed-by: John Daley <johndale@cisco.com>
>>> ---
>>>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/net/enic/enic_ethdev.c
>> b/drivers/net/enic/enic_ethdev.c
>>> index 06dc67122..85d785e62 100644
>>> --- a/drivers/net/enic/enic_ethdev.c
>>> +++ b/drivers/net/enic/enic_ethdev.c
>>> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev *dev,
>>>  {
>>>  	int ret = 0;
>>>
>>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>> +		return -E_RTE_SECONDARY;
>>>  	ENICPMD_FUNC_TRACE();
>>>
>>>  	switch (filter_type) {
>>
>> I remember we have similar talk with John in the past about these secondary
>> application checks in ethdev_ops.
>>
>> I would like to understand why these checks required only in enic, can you
>> please describe your use case?
>> Is there any reason secondary application can't change configuration of the
>> device, or are you updating your driver to work with specific application?
> 
> Hi,
> 
> No fundamental reasons why secondary processes cannot run these
> handlers. These checks are to make it clear that it is not safe to do
> so at the moment. It is a software limitation.
> 
> The firmware API (devcmd) we use to configure NIC settings assumes one
> user executing one command at a time. And, many of the handlers in the
> driver also assume primary process. The firmware itself has
> enough checks to prevent concurrent devcmd attempts from corrupting
> its internal state. But, host processes can get confused. For example,
> process A gets process B's results, or overwrites B's devcmd, etc.
> 
> I believe these issues are all fixable in the driver. We could use
> locks in shared memory to serialize devcmd (though risky), fix
> handlers that assume primary process, and so on. It is a to-do item for
> this driver and would require its own patch series (e.g. allow
> secondary processes to run X, Y, Z safely)..

What you have described is valid concern for all drivers, that synchronization
has been pushed to the application level.

I don't see the point of just putting protection to only one driver.

And as a alternative, what do you think about having a check in the prob for the
secondary process and assign a subset of the ethdev_ops in that case? This makes
 more clear what is supported in the secondary process, and it prevents putting
secondary process checks everywhere.

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to primary process

[Hyong Youb Kim (hyonkim)]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 9, 2019 6:28 PM
> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to
> primary process
> 
> On 10/9/2019 9:48 AM, Hyong Youb Kim (hyonkim) wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Wednesday, October 9, 2019 5:02 PM
> >> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> >> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
> >> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
> >> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
> >> process
> >>
> >> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
> >>> These eth_dev_ops handlers should run only in the primary process.
> >>> - filter_ctrl
> >>> - reta_update
> >>> - rss_hash_update
> >>> - set_mc_addr_list
> >>> - udp_tunnel_port_add
> >>> - udp_tunnel_port_del
> >>>
> >>> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
> >>> Fixes: 8d496995346c ("net/enic: support multicast filtering")
> >>> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
> >> number")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> >>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> >>> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
> >>> Reviewed-by: John Daley <johndale@cisco.com>
> >>> ---
> >>>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/net/enic/enic_ethdev.c
> >> b/drivers/net/enic/enic_ethdev.c
> >>> index 06dc67122..85d785e62 100644
> >>> --- a/drivers/net/enic/enic_ethdev.c
> >>> +++ b/drivers/net/enic/enic_ethdev.c
> >>> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
> *dev,
> >>>  {
> >>>  	int ret = 0;
> >>>
> >>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>> +		return -E_RTE_SECONDARY;
> >>>  	ENICPMD_FUNC_TRACE();
> >>>
> >>>  	switch (filter_type) {
> >>
> >> I remember we have similar talk with John in the past about these
> secondary
> >> application checks in ethdev_ops.
> >>
> >> I would like to understand why these checks required only in enic, can you
> >> please describe your use case?
> >> Is there any reason secondary application can't change configuration of
> the
> >> device, or are you updating your driver to work with specific application?
> >
> > Hi,
> >
> > No fundamental reasons why secondary processes cannot run these
> > handlers. These checks are to make it clear that it is not safe to do
> > so at the moment. It is a software limitation.
> >
> > The firmware API (devcmd) we use to configure NIC settings assumes one
> > user executing one command at a time. And, many of the handlers in the
> > driver also assume primary process. The firmware itself has
> > enough checks to prevent concurrent devcmd attempts from corrupting
> > its internal state. But, host processes can get confused. For example,
> > process A gets process B's results, or overwrites B's devcmd, etc.
> >
> > I believe these issues are all fixable in the driver. We could use
> > locks in shared memory to serialize devcmd (though risky), fix
> > handlers that assume primary process, and so on. It is a to-do item for
> > this driver and would require its own patch series (e.g. allow
> > secondary processes to run X, Y, Z safely)..
> 
> What you have described is valid concern for all drivers, that synchronization
> has been pushed to the application level.
> 
> I don't see the point of just putting protection to only one driver.
> 
> And as a alternative, what do you think about having a check in the prob for
> the
> secondary process and assign a subset of the ethdev_ops in that case? This
> makes
>  more clear what is supported in the secondary process, and it prevents
> putting
> secondary process checks everywhere.

Hi,

Okay, that sounds reasonable. Could you drop this one patch and apply
the rest in the series? I may not have time to properly re-do this one
in this cycle..

Thanks!
-Hyong

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to primary process

[Ferruh Yigit]

On 10/9/2019 10:38 AM, Hyong Youb Kim (hyonkim) wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, October 9, 2019 6:28 PM
>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH 1/3] net/enic: restrict several handlers to
>> primary process
>>
>> On 10/9/2019 9:48 AM, Hyong Youb Kim (hyonkim) wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: Wednesday, October 9, 2019 5:02 PM
>>>> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>>>> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>; Dirk-
>>>> Holger Lenz <dirk.lenz@ng4t.com>; stable@dpdk.org
>>>> Subject: Re: [PATCH 1/3] net/enic: restrict several handlers to primary
>>>> process
>>>>
>>>> On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
>>>>> These eth_dev_ops handlers should run only in the primary process.
>>>>> - filter_ctrl
>>>>> - reta_update
>>>>> - rss_hash_update
>>>>> - set_mc_addr_list
>>>>> - udp_tunnel_port_add
>>>>> - udp_tunnel_port_del
>>>>>
>>>>> Fixes: c2fec27b5cb0 ("net/enic: allow to change RSS settings")
>>>>> Fixes: 8d496995346c ("net/enic: support multicast filtering")
>>>>> Fixes: 8a4efd17410c ("net/enic: add handlers to add/delete vxlan port
>>>> number")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>>>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
>>>>> Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
>>>>> Reviewed-by: John Daley <johndale@cisco.com>
>>>>> ---
>>>>>  drivers/net/enic/enic_ethdev.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/enic/enic_ethdev.c
>>>> b/drivers/net/enic/enic_ethdev.c
>>>>> index 06dc67122..85d785e62 100644
>>>>> --- a/drivers/net/enic/enic_ethdev.c
>>>>> +++ b/drivers/net/enic/enic_ethdev.c
>>>>> @@ -129,6 +129,8 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
>> *dev,
>>>>>  {
>>>>>  	int ret = 0;
>>>>>
>>>>> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>> +		return -E_RTE_SECONDARY;
>>>>>  	ENICPMD_FUNC_TRACE();
>>>>>
>>>>>  	switch (filter_type) {
>>>>
>>>> I remember we have similar talk with John in the past about these
>> secondary
>>>> application checks in ethdev_ops.
>>>>
>>>> I would like to understand why these checks required only in enic, can you
>>>> please describe your use case?
>>>> Is there any reason secondary application can't change configuration of
>> the
>>>> device, or are you updating your driver to work with specific application?
>>>
>>> Hi,
>>>
>>> No fundamental reasons why secondary processes cannot run these
>>> handlers. These checks are to make it clear that it is not safe to do
>>> so at the moment. It is a software limitation.
>>>
>>> The firmware API (devcmd) we use to configure NIC settings assumes one
>>> user executing one command at a time. And, many of the handlers in the
>>> driver also assume primary process. The firmware itself has
>>> enough checks to prevent concurrent devcmd attempts from corrupting
>>> its internal state. But, host processes can get confused. For example,
>>> process A gets process B's results, or overwrites B's devcmd, etc.
>>>
>>> I believe these issues are all fixable in the driver. We could use
>>> locks in shared memory to serialize devcmd (though risky), fix
>>> handlers that assume primary process, and so on. It is a to-do item for
>>> this driver and would require its own patch series (e.g. allow
>>> secondary processes to run X, Y, Z safely)..
>>
>> What you have described is valid concern for all drivers, that synchronization
>> has been pushed to the application level.
>>
>> I don't see the point of just putting protection to only one driver.
>>
>> And as a alternative, what do you think about having a check in the prob for
>> the
>> secondary process and assign a subset of the ethdev_ops in that case? This
>> makes
>>  more clear what is supported in the secondary process, and it prevents
>> putting
>> secondary process checks everywhere.
> 
> Hi,
> 
> Okay, that sounds reasonable. Could you drop this one patch and apply
> the rest in the series? I may not have time to properly re-do this one
> in this cycle..

OK, will mark 1/3 as "Change Requested" and continue with others. Thanks.

[dpdk-dev] [PATCH 2/3] net/enic: fix two issues in probe for secondary process

[Hyong Youb Kim]

Only the primary process initializes the adapter private data and
rte_eth_dev_data. For secondary processes, do not touch them.

Secondary processes need to select the right Tx and Rx handlers. Pick
the same handlers that the primary process uses.

Fixes: fefed3d1e62c ("enic: new driver")
Cc: stable@dpdk.org

Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic.h               |  5 ++++-
 drivers/net/enic/enic_ethdev.c        | 10 ++++++++--
 drivers/net/enic/enic_main.c          | 28 ++++++++++++++++++++-------
 drivers/net/enic/enic_rxtx_vec_avx2.c |  5 ++---
 4 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 5a92508f0..2c8b3e0e4 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -134,6 +134,7 @@ struct enic {
 	bool udp_rss_weak;    /* Bodega style UDP RSS */
 	uint8_t ig_vlan_rewrite_mode; /* devargs ig-vlan-rewrite */
 	uint16_t vxlan_port;  /* current vxlan port pushed to NIC */
+	int use_simple_tx_handler;
 
 	unsigned int flags;
 	unsigned int priv_flags;
@@ -333,7 +334,9 @@ uint16_t enic_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 			uint16_t nb_pkts);
 int enic_set_mtu(struct enic *enic, uint16_t new_mtu);
 int enic_link_update(struct enic *enic);
-bool enic_use_vector_rx_handler(struct enic *enic);
+bool enic_use_vector_rx_handler(struct rte_eth_dev *eth_dev);
+void enic_pick_rx_handler(struct rte_eth_dev *eth_dev);
+void enic_pick_tx_handler(struct rte_eth_dev *eth_dev);
 void enic_fdir_info(struct enic *enic);
 void enic_fdir_info_get(struct enic *enic, struct rte_eth_fdir_info *stats);
 extern const struct rte_flow_ops enic_flow_ops;
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 85d785e62..c6d0d7557 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1216,12 +1216,18 @@ static int eth_enicpmd_dev_init(struct rte_eth_dev *eth_dev)
 
 	ENICPMD_FUNC_TRACE();
 
-	enic->port_id = eth_dev->data->port_id;
-	enic->rte_dev = eth_dev;
 	eth_dev->dev_ops = &enicpmd_eth_dev_ops;
 	eth_dev->rx_pkt_burst = &enic_recv_pkts;
 	eth_dev->tx_pkt_burst = &enic_xmit_pkts;
 	eth_dev->tx_pkt_prepare = &enic_prep_pkts;
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		enic_pick_tx_handler(eth_dev);
+		enic_pick_rx_handler(eth_dev);
+		return 0;
+	}
+	/* Only the primary sets up adapter and other data in shared memory */
+	enic->port_id = eth_dev->data->port_id;
+	enic->rte_dev = eth_dev;
 	/* Let rte_eth_dev_close() release the port resources */
 	eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 40af3781b..aecdee248 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -518,14 +518,14 @@ static void enic_prep_wq_for_simple_tx(struct enic *enic, uint16_t queue_idx)
  * used when that file is not compiled.
  */
 __rte_weak bool
-enic_use_vector_rx_handler(__rte_unused struct enic *enic)
+enic_use_vector_rx_handler(__rte_unused struct rte_eth_dev *eth_dev)
 {
 	return false;
 }
 
-static void pick_rx_handler(struct enic *enic)
+void enic_pick_rx_handler(struct rte_eth_dev *eth_dev)
 {
-	struct rte_eth_dev *eth_dev;
+	struct enic *enic = pmd_priv(eth_dev);
 
 	/*
 	 * Preference order:
@@ -533,8 +533,7 @@ static void pick_rx_handler(struct enic *enic)
 	 * 2. The non-scatter, simplified handler if scatter Rx is not used.
 	 * 3. The default handler as a fallback.
 	 */
-	eth_dev = enic->rte_dev;
-	if (enic_use_vector_rx_handler(enic))
+	if (enic_use_vector_rx_handler(eth_dev))
 		return;
 	if (enic->rq_count > 0 && enic->rq[0].data_queue_enable == 0) {
 		ENICPMD_LOG(DEBUG, " use the non-scatter Rx handler");
@@ -545,6 +544,20 @@ static void pick_rx_handler(struct enic *enic)
 	}
 }
 
+/* Secondary process uses this to set the Tx handler */
+void enic_pick_tx_handler(struct rte_eth_dev *eth_dev)
+{
+	struct enic *enic = pmd_priv(eth_dev);
+
+	if (enic->use_simple_tx_handler) {
+		ENICPMD_LOG(DEBUG, " use the simple tx handler");
+		eth_dev->tx_pkt_burst = &enic_simple_xmit_pkts;
+	} else {
+		ENICPMD_LOG(DEBUG, " use the default tx handler");
+		eth_dev->tx_pkt_burst = &enic_xmit_pkts;
+	}
+}
+
 int enic_enable(struct enic *enic)
 {
 	unsigned int index;
@@ -622,12 +635,13 @@ int enic_enable(struct enic *enic)
 		eth_dev->tx_pkt_burst = &enic_simple_xmit_pkts;
 		for (index = 0; index < enic->wq_count; index++)
 			enic_prep_wq_for_simple_tx(enic, index);
+		enic->use_simple_tx_handler = 1;
 	} else {
 		ENICPMD_LOG(DEBUG, " use the default tx handler");
 		eth_dev->tx_pkt_burst = &enic_xmit_pkts;
 	}
 
-	pick_rx_handler(enic);
+	enic_pick_rx_handler(eth_dev);
 
 	for (index = 0; index < enic->wq_count; index++)
 		enic_start_wq(enic, index);
@@ -1598,7 +1612,7 @@ int enic_set_mtu(struct enic *enic, uint16_t new_mtu)
 
 	/* put back the real receive function */
 	rte_mb();
-	pick_rx_handler(enic);
+	enic_pick_rx_handler(eth_dev);
 	rte_mb();
 
 	/* restart Rx traffic */
diff --git a/drivers/net/enic/enic_rxtx_vec_avx2.c b/drivers/net/enic/enic_rxtx_vec_avx2.c
index 517d4092f..36d4d0dea 100644
--- a/drivers/net/enic/enic_rxtx_vec_avx2.c
+++ b/drivers/net/enic/enic_rxtx_vec_avx2.c
@@ -806,12 +806,11 @@ enic_noscatter_vec_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 }
 
 bool
-enic_use_vector_rx_handler(struct enic *enic)
+enic_use_vector_rx_handler(struct rte_eth_dev *eth_dev)
 {
-	struct rte_eth_dev *eth_dev;
+	struct enic *enic = pmd_priv(eth_dev);
 	struct rte_fdir_conf *fconf;
 
-	eth_dev = enic->rte_dev;
 	/* User needs to request for the avx2 handler */
 	if (!enic->enable_avx2_rx)
 		return false;
-- 
2.22.0

[dpdk-dev] [PATCH 3/3] net/enic: fix crash in secondary process

[Hyong Youb Kim]

Both primary and secondary processes may call the queue start/stop,
link update handlers. These functions use the rte_eth_dev pointer
cached in the adapter private data (struct enic). But, this pointer is
valid only in the primary process, as rte_eth_dev addresses may differ
in different processes. Using that cached pointer in secondary
processes leads to a crash.

For the link update handler (enic_link_update), use the rte_eth_dev
pointer passed down from the rte layer as it is valid in the current
process. For the queue start/stop handlers (enic_start_wq and
friends), cache the rte_eth_dev_data pointer in the adapter private
data, and use that. rte_eth_dev_data is in shared memory and its
address is same across processes.

Fixes: 837e68ae94a2 ("net/enic: fix queue stop and start")
Fixes: cf8d9826b7be ("net/enic: extract code for checking link status")
Cc: stable@dpdk.org

Reported-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
Tested-by: Dirk-Holger Lenz <dirk.lenz@ng4t.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic.h        |  3 ++-
 drivers/net/enic/enic_ethdev.c |  5 ++---
 drivers/net/enic/enic_main.c   | 22 +++++++++++-----------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 2c8b3e0e4..4a2e3024f 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -110,6 +110,7 @@ struct enic {
 	unsigned int port_id;
 	bool overlay_offload;
 	struct rte_eth_dev *rte_dev;
+	struct rte_eth_dev_data *dev_data;
 	struct enic_fdir fdir;
 	char bdf_name[ENICPMD_BDF_LENGTH];
 	int dev_fd;
@@ -333,7 +334,7 @@ uint16_t enic_simple_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 uint16_t enic_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
 			uint16_t nb_pkts);
 int enic_set_mtu(struct enic *enic, uint16_t new_mtu);
-int enic_link_update(struct enic *enic);
+int enic_link_update(struct rte_eth_dev *eth_dev);
 bool enic_use_vector_rx_handler(struct rte_eth_dev *eth_dev);
 void enic_pick_rx_handler(struct rte_eth_dev *eth_dev);
 void enic_pick_tx_handler(struct rte_eth_dev *eth_dev);
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index c6d0d7557..54b939744 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -461,10 +461,8 @@ static void enicpmd_dev_close(struct rte_eth_dev *eth_dev)
 static int enicpmd_dev_link_update(struct rte_eth_dev *eth_dev,
 	__rte_unused int wait_to_complete)
 {
-	struct enic *enic = pmd_priv(eth_dev);
-
 	ENICPMD_FUNC_TRACE();
-	return enic_link_update(enic);
+	return enic_link_update(eth_dev);
 }
 
 static int enicpmd_dev_stats_get(struct rte_eth_dev *eth_dev,
@@ -1228,6 +1226,7 @@ static int eth_enicpmd_dev_init(struct rte_eth_dev *eth_dev)
 	/* Only the primary sets up adapter and other data in shared memory */
 	enic->port_id = eth_dev->data->port_id;
 	enic->rte_dev = eth_dev;
+	enic->dev_data = eth_dev->data;
 	/* Let rte_eth_dev_close() release the port resources */
 	eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index aecdee248..63c85069a 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -417,9 +417,9 @@ enic_free_consistent(void *priv,
 	rte_free(mze);
 }
 
-int enic_link_update(struct enic *enic)
+int enic_link_update(struct rte_eth_dev *eth_dev)
 {
-	struct rte_eth_dev *eth_dev = enic->rte_dev;
+	struct enic *enic = pmd_priv(eth_dev);
 	struct rte_eth_link link;
 
 	memset(&link, 0, sizeof(link));
@@ -438,7 +438,7 @@ enic_intr_handler(void *arg)
 
 	vnic_intr_return_all_credits(&enic->intr[ENICPMD_LSC_INTR_OFFSET]);
 
-	enic_link_update(enic);
+	enic_link_update(dev);
 	_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL);
 	enic_log_q_error(enic);
 }
@@ -731,31 +731,31 @@ void enic_free_rq(void *rxq)
 
 void enic_start_wq(struct enic *enic, uint16_t queue_idx)
 {
-	struct rte_eth_dev *eth_dev = enic->rte_dev;
+	struct rte_eth_dev_data *data = enic->dev_data;
 	vnic_wq_enable(&enic->wq[queue_idx]);
-	eth_dev->data->tx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STARTED;
+	data->tx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STARTED;
 }
 
 int enic_stop_wq(struct enic *enic, uint16_t queue_idx)
 {
-	struct rte_eth_dev *eth_dev = enic->rte_dev;
+	struct rte_eth_dev_data *data = enic->dev_data;
 	int ret;
 
 	ret = vnic_wq_disable(&enic->wq[queue_idx]);
 	if (ret)
 		return ret;
 
-	eth_dev->data->tx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STOPPED;
+	data->tx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STOPPED;
 	return 0;
 }
 
 void enic_start_rq(struct enic *enic, uint16_t queue_idx)
 {
+	struct rte_eth_dev_data *data = enic->dev_data;
 	struct vnic_rq *rq_sop;
 	struct vnic_rq *rq_data;
 	rq_sop = &enic->rq[enic_rte_rq_idx_to_sop_idx(queue_idx)];
 	rq_data = &enic->rq[rq_sop->data_queue_idx];
-	struct rte_eth_dev *eth_dev = enic->rte_dev;
 
 	if (rq_data->in_use) {
 		vnic_rq_enable(rq_data);
@@ -764,13 +764,13 @@ void enic_start_rq(struct enic *enic, uint16_t queue_idx)
 	rte_mb();
 	vnic_rq_enable(rq_sop);
 	enic_initial_post_rx(enic, rq_sop);
-	eth_dev->data->rx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STARTED;
+	data->rx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STARTED;
 }
 
 int enic_stop_rq(struct enic *enic, uint16_t queue_idx)
 {
+	struct rte_eth_dev_data *data = enic->dev_data;
 	int ret1 = 0, ret2 = 0;
-	struct rte_eth_dev *eth_dev = enic->rte_dev;
 	struct vnic_rq *rq_sop;
 	struct vnic_rq *rq_data;
 	rq_sop = &enic->rq[enic_rte_rq_idx_to_sop_idx(queue_idx)];
@@ -786,7 +786,7 @@ int enic_stop_rq(struct enic *enic, uint16_t queue_idx)
 	else if (ret1)
 		return ret1;
 
-	eth_dev->data->rx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STOPPED;
+	data->rx_queue_state[queue_idx] = RTE_ETH_QUEUE_STATE_STOPPED;
 	return 0;
 }
 
-- 
2.22.0

Re: [dpdk-dev] [PATCH 0/3] net/enic: fix multi-process isseus

[Ferruh Yigit]

On 9/6/2019 7:50 AM, Hyong Youb Kim wrote:
> This series fixes a few multi-process issues, mainly discovered by
> Dirk-Holger Lenz. They manifest clearly when primary and secondary
> processes use different executable images (hence different rte_eth_dev
> addresses and so on).
> 
> Dirk has tested this series, and his app works fine. Below is the
> original email thread.
> http://mails.dpdk.org/archives/dev/2019-August/141560.html
> 
> This series replaces the earlier one.
> http://patches.dpdk.org/patch/58033/
> 
> Thanks.
> -Hyong
> 
> Hyong Youb Kim (3):
>   net/enic: restrict several handlers to primary process
>   net/enic: fix two issues in probe for secondary process
>   net/enic: fix crash in secondary process

Series applied to dpdk-next-net/master, except 1/3.

Change requested for 1/3.