[dpdk-dev] [PATCH] ethdev: fix secondary process change share memory

[dpdk-dev] [PATCH] ethdev: fix secondary process change share memory

[Fang TongHao]

Hi all，I am from Sangfor Tech.I found a bug when using DPDK in
multiprocess scenario.The secondary process enters
"rte_eth_dev_pci_copy_info" function when initializing.Then it
sets the value of struct "rte_eth_dev_data.dev_flags" to zero,
but this struct is shared by primary process and secondary
process, and the value change is unexpected by primary process.
This may cause very serious damage.I think
the secondary process should not enter "rte_eth_dev_pci_copy_info"
function or changes the value of struct "rte_eth_dev_data.dev_flags"
in shared memory.
I fixed this bug by adding an if-statement to forbid the secondary
process changing the above-mentioned value.
Thansk, All.

Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
---
 lib/librte_ethdev/rte_ethdev_pci.h | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
index ccdbb46ec..916de8a14 100644
--- a/lib/librte_ethdev/rte_ethdev_pci.h
+++ b/lib/librte_ethdev/rte_ethdev_pci.h
@@ -59,15 +59,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
 	}
 
 	eth_dev->intr_handle = &pci_dev->intr_handle;
-
-	eth_dev->data->dev_flags = 0;
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
-		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
-		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
-
-	eth_dev->data->kdrv = pci_dev->kdrv;
-	eth_dev->data->numa_node = pci_dev->device.numa_node;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev->data->dev_flags = 0;
+		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
+			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
+		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
+			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
+
+		eth_dev->data->kdrv = pci_dev->kdrv;
+		eth_dev->data->numa_node = pci_dev->device.numa_node;
+	}
 }
 
 static inline int
-- 
2.24.1.windows.2

Re: [dpdk-dev] [PATCH] ethdev: fix secondary process change share memory

[Jeff Guo]

hi, tonghao


On 1/9/2020 8:27 PM, Fang TongHao wrote:
> Hi all，I am from Sangfor Tech.I found a bug when using DPDK in
> multiprocess scenario.The secondary process enters
> "rte_eth_dev_pci_copy_info" function when initializing.Then it
> sets the value of struct "rte_eth_dev_data.dev_flags" to zero,
> but this struct is shared by primary process and secondary
> process, and the value change is unexpected by primary process.
> This may cause very serious damage.I think
> the secondary process should not enter "rte_eth_dev_pci_copy_info"
> function or changes the value of struct "rte_eth_dev_data.dev_flags"
> in shared memory.
> I fixed this bug by adding an if-statement to forbid the secondary
> process changing the above-mentioned value.
> Thansk, All.


i think the format of commit log should be refined to be more formal 
like as below. what do you think?

ethdev: XXXXXXXXX

XXXXXXXX


> Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>


if it is a fix, suggest to add the line as "Fixes: XXXXXXXX ("ethdev: 
XXXXXXX") to trace it.


> ---
>   lib/librte_ethdev/rte_ethdev_pci.h | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index ccdbb46ec..916de8a14 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -59,15 +59,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
>   	}
>   
>   	eth_dev->intr_handle = &pci_dev->intr_handle;
> -
> -	eth_dev->data->dev_flags = 0;
> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
> -
> -	eth_dev->data->kdrv = pci_dev->kdrv;
> -	eth_dev->data->numa_node = pci_dev->device.numa_node;
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		eth_dev->data->dev_flags = 0;
> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
> +
> +		eth_dev->data->kdrv = pci_dev->kdrv;
> +		eth_dev->data->numa_node = pci_dev->device.numa_node;


 From the change log, you said that "rte_eth_dev_data.dev_flags" should 
not be touched by secondary process, but you don't mention about

data->kdrv and data->numa_node, could you also explain them in the log 
if they need to process as the same.


> +	}
>   }
>   
>   static inline int

Re: [dpdk-dev] [PATCH] ethdev: fix secondary process change share memory

[方统浩50450]

thanks for your correction 
I will rewrite my commit log and send email again


方统浩50450
邮箱：fangtonghao@sangfor.com.cn
签名由 网易邮箱大师 定制


On 01/10/2020 15:30, Jeff Guo wrote:

hi, tonghao


 On 1/9/2020 8:27 PM, Fang TongHao wrote:
 > Hi all，I am from Sangfor Tech.I found a bug when using DPDK in
 > multiprocess scenario.The secondary process enters
 > "rte_eth_dev_pci_copy_info" function when initializing.Then it
 > sets the value of struct "rte_eth_dev_data.dev_flags" to zero,
 > but this struct is shared by primary process and secondary
 > process, and the value change is unexpected by primary process.
 > This may cause very serious damage.I think
 > the secondary process should not enter "rte_eth_dev_pci_copy_info"
 > function or changes the value of struct "rte_eth_dev_data.dev_flags"
 > in shared memory.
 > I fixed this bug by adding an if-statement to forbid the secondary
 > process changing the above-mentioned value.
 > Thansk, All.


 i think the format of commit log should be refined to be more formal
 like as below. what do you think?

 ethdev: XXXXXXXXX

 XXXXXXXX


 > Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>


 if it is a fix, suggest to add the line as "Fixes: XXXXXXXX ("ethdev:
 XXXXXXX") to trace it.


 > ---
 >   lib/librte_ethdev/rte_ethdev_pci.h | 19 ++++++++++---------
 >   1 file changed, 10 insertions(+), 9 deletions(-)
 >
 > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
 > index ccdbb46ec..916de8a14 100644
 > --- a/lib/librte_ethdev/rte_ethdev_pci.h
 > +++ b/lib/librte_ethdev/rte_ethdev_pci.h
 > @@ -59,15 +59,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
 >       }
 >   
 >       eth_dev->intr_handle = &pci_dev->intr_handle;
 > -
 > -    eth_dev->data->dev_flags = 0;
 > -    if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
 > -        eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
 > -    if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
 > -        eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
 > -
 > -    eth_dev->data->kdrv = pci_dev->kdrv;
 > -    eth_dev->data->numa_node = pci_dev->device.numa_node;
 > +    if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 > +        eth_dev->data->dev_flags = 0;
 > +        if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
 > +            eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
 > +        if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
 > +            eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
 > +
 > +        eth_dev->data->kdrv = pci_dev->kdrv;
 > +        eth_dev->data->numa_node = pci_dev->device.numa_node;


 From the change log, you said that "rte_eth_dev_data.dev_flags" should
 not be touched by secondary process, but you don't mention about

 data->kdrv and data->numa_node, could you also explain them in the log
 if they need to process as the same.


 > +    }
 >   }
 >   
 >   static inline int

[dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Fang TongHao]

Secondary process calls “rte_eth_dev_pci_allocate”
function and enters rte_eth_copy_pci_info function
when initializing.Then it sets the value of struct
"rte_eth_dev_data.dev_flags" to zero and reset it,
but this struct is shared by primary process and
secondary process.To fix this bug,by adding an
if-statement to forbid the secondaryprocess changing
the above-mentioned value.

Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
---
 lib/librte_ethdev/rte_ethdev_pci.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
index ccdbb46ec..e7dae0545 100644
--- a/lib/librte_ethdev/rte_ethdev_pci.h
+++ b/lib/librte_ethdev/rte_ethdev_pci.h
@@ -60,14 +60,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
 
 	eth_dev->intr_handle = &pci_dev->intr_handle;
 
-	eth_dev->data->dev_flags = 0;
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
-		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
-		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
-
-	eth_dev->data->kdrv = pci_dev->kdrv;
-	eth_dev->data->numa_node = pci_dev->device.numa_node;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev->data->dev_flags = 0;
+		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
+			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
+		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
+			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
+
+		eth_dev->data->kdrv = pci_dev->kdrv;
+		eth_dev->data->numa_node = pci_dev->device.numa_node;
+	}
 }
 
 static inline int
-- 
2.24.1.windows.2

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Ferruh Yigit]

On 1/13/2020 5:03 AM, Fang TongHao wrote:
> Secondary process calls “rte_eth_dev_pci_allocate”
> function and enters rte_eth_copy_pci_info function
> when initializing.Then it sets the value of struct
> "rte_eth_dev_data.dev_flags" to zero and reset it,
> but this struct is shared by primary process and
> secondary process.To fix this bug,by adding an
> if-statement to forbid the secondaryprocess changing
> the above-mentioned value.

Hi Fang,

Thanks for the fix, I agree with the problem statement, but not sure if this
should be handled in the helper function or in the place where the function is
called. Helper function is simple on what it does, do we need to put the primary
process logic in it.

Can you please give more details of the bug you have encounter, is it seen by a
specific PMD?

Thanks,
ferruh

> 
> Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
> ---
>  lib/librte_ethdev/rte_ethdev_pci.h | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index ccdbb46ec..e7dae0545 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -60,14 +60,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
>  
>  	eth_dev->intr_handle = &pci_dev->intr_handle;
>  
> -	eth_dev->data->dev_flags = 0;
> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
> -
> -	eth_dev->data->kdrv = pci_dev->kdrv;
> -	eth_dev->data->numa_node = pci_dev->device.numa_node;
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		eth_dev->data->dev_flags = 0;
> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
> +
> +		eth_dev->data->kdrv = pci_dev->kdrv;
> +		eth_dev->data->numa_node = pci_dev->device.numa_node;
> +	}
>  }
>  
>  static inline int
> 

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[方统浩50450]

Hi Ferruh, thanks for your message.


We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
secondary process will change the shared memory when initializing.Secondary process calls
"rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
(rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
process.


Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
function should simple on what it does.I have two ways to fix this problem, one is add an if-statement

in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
I think the second way is simple and lower risk.


Please forgive me because my poor english....



发件人：Ferruh Yigit <ferruh.yigit@intel.com>
发送日期：2020-01-14 22:45:33
收件人：Fang TongHao <fangtonghao@sangfor.com.cn>,thomas@monjalon.net,arybchenko@solarflare.com
抄送人：dev@dpdk.org,stable@dpdk.org,jia.guo@intel.com,cunming.liang@intel.com,qi.z.zhang@intel.com,jungle845943968@outlook.com
主题：Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory>On 1/13/2020 5:03 AM, Fang TongHao wrote:
>> Secondary process calls “rte_eth_dev_pci_allocate”
>> function and enters rte_eth_copy_pci_info function
>> when initializing.Then it sets the value of struct
>> "rte_eth_dev_data.dev_flags" to zero and reset it,
>> but this struct is shared by primary process and
>> secondary process.To fix this bug,by adding an
>> if-statement to forbid the secondaryprocess changing
>> the above-mentioned value.
>
>Hi Fang,
>
>Thanks for the fix, I agree with the problem statement, but not sure if this
>should be handled in the helper function or in the place where the function is
>called. Helper function is simple on what it does, do we need to put the primary
>process logic in it.
>
>Can you please give more details of the bug you have encounter, is it seen by a
>specific PMD?
>
>Thanks,
>ferruh
>
>> 
>> Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
>> ---
>>  lib/librte_ethdev/rte_ethdev_pci.h | 18 ++++++++++--------
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>> 
>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
>> index ccdbb46ec..e7dae0545 100644
>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
>> @@ -60,14 +60,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
>>  
>>  	eth_dev->intr_handle = &pci_dev->intr_handle;
>>  
>> -	eth_dev->data->dev_flags = 0;
>> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
>> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
>> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
>> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
>> -
>> -	eth_dev->data->kdrv = pci_dev->kdrv;
>> -	eth_dev->data->numa_node = pci_dev->device.numa_node;
>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>> +		eth_dev->data->dev_flags = 0;
>> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
>> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
>> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
>> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
>> +
>> +		eth_dev->data->kdrv = pci_dev->kdrv;
>> +		eth_dev->data->numa_node = pci_dev->device.numa_node;
>> +	}
>>  }
>>  
>>  static inline int
>> 
>

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Ferruh Yigit]

On 1/15/2020 6:49 AM, 方统浩50450 wrote:
> Hi Ferruh, thanks for your message.
> 
> 
> We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
> support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
> secondary process will change the shared memory when initializing.Secondary process calls
> "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
> (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
> Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
> is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
> the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
> detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
> the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
> process.

I agree this is the problem.
In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
but the generic code is faulty.

And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.

> 
> 
> Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
> enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
> function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
> 
> in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
> another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
> shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
> I think the second way is simple and lower risk.

Yes these are the two options.

I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
BUT my concern was adding decision making to simple/leaf function and make it
harder to debug/use, instead of giving what primary/secondary process should
call decision in higher level.

But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
secondary process, like mlx4 or szedata2, and most probably this is not their
intention.
And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
function may have side affect of 'eth_dev->intr_handle' not set in secondary.

With above considerations I am OK to your proposal to cover all cases, Thomas,
Andrew, any concern?

@Fang, only can you please make a new version to update the
'rte_eth_copy_pci_info' function comment to document shared data is not updated
for the secondary process?

Thanks,
ferruh

> 
> 
> Please forgive me because my poor english....
> 
> 
> 
> 发件人：Ferruh Yigit <ferruh.yigit@intel.com>
> 发送日期：2020-01-14 22:45:33
> 收件人：Fang TongHao <fangtonghao@sangfor.com.cn>,thomas@monjalon.net,arybchenko@solarflare.com
> 抄送人：dev@dpdk.org,stable@dpdk.org,jia.guo@intel.com,cunming.liang@intel.com,qi.z.zhang@intel.com,jungle845943968@outlook.com
> 主题：Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory>On 1/13/2020 5:03 AM, Fang TongHao wrote:
>>> Secondary process calls “rte_eth_dev_pci_allocate”
>>> function and enters rte_eth_copy_pci_info function
>>> when initializing.Then it sets the value of struct
>>> "rte_eth_dev_data.dev_flags" to zero and reset it,
>>> but this struct is shared by primary process and
>>> secondary process.To fix this bug,by adding an
>>> if-statement to forbid the secondaryprocess changing
>>> the above-mentioned value.
>>
>> Hi Fang,
>>
>> Thanks for the fix, I agree with the problem statement, but not sure if this
>> should be handled in the helper function or in the place where the function is
>> called. Helper function is simple on what it does, do we need to put the primary
>> process logic in it.
>>
>> Can you please give more details of the bug you have encounter, is it seen by a
>> specific PMD?
>>
>> Thanks,
>> ferruh
>>
>>>
>>> Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
>>> ---
>>>  lib/librte_ethdev/rte_ethdev_pci.h | 18 ++++++++++--------
>>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
>>> index ccdbb46ec..e7dae0545 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_pci.h
>>> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
>>> @@ -60,14 +60,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
>>>  
>>>  	eth_dev->intr_handle = &pci_dev->intr_handle;
>>>  
>>> -	eth_dev->data->dev_flags = 0;
>>> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
>>> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
>>> -	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
>>> -		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
>>> -
>>> -	eth_dev->data->kdrv = pci_dev->kdrv;
>>> -	eth_dev->data->numa_node = pci_dev->device.numa_node;
>>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>>> +		eth_dev->data->dev_flags = 0;
>>> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
>>> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
>>> +		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
>>> +			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
>>> +
>>> +		eth_dev->data->kdrv = pci_dev->kdrv;
>>> +		eth_dev->data->numa_node = pci_dev->device.numa_node;
>>> +	}
>>>  }
>>>  
>>>  static inline int
>>>
>>
> 
> 
> 
> 

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Thomas Monjalon]

15/01/2020 19:35, Ferruh Yigit:
> On 1/15/2020 6:49 AM, 方统浩50450 wrote:
> > Hi Ferruh, thanks for your message.
> > 
> > 
> > We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
> > support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
> > secondary process will change the shared memory when initializing.Secondary process calls
> > "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
> > (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
> > Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
> > is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
> > the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
> > detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
> > the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
> > process.
> 
> I agree this is the problem.
> In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
> but the generic code is faulty.
> 
> And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.
> 
> > Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
> > enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
> > function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
> > 
> > in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
> > another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
> > shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
> > I think the second way is simple and lower risk.
> 
> Yes these are the two options.
> 
> I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
> BUT my concern was adding decision making to simple/leaf function and make it
> harder to debug/use, instead of giving what primary/secondary process should
> call decision in higher level.
> 
> But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
> secondary process, like mlx4 or szedata2, and most probably this is not their
> intention.
> And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
> function may have side affect of 'eth_dev->intr_handle' not set in secondary.
> 
> With above considerations I am OK to your proposal to cover all cases, Thomas,
> Andrew, any concern?

Do you mean drivers need to be fixed?

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Andrew Rybchenko]

On 1/15/20 11:43 PM, Thomas Monjalon wrote:
> 15/01/2020 19:35, Ferruh Yigit:
>> On 1/15/2020 6:49 AM, 方统浩50450 wrote:
>>> Hi Ferruh, thanks for your message.
>>>
>>>
>>> We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
>>> support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
>>> secondary process will change the shared memory when initializing.Secondary process calls
>>> "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
>>> (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
>>> Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
>>> is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
>>> the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
>>> detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
>>> the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
>>> process.

Hold on, just for my understanding. As far as I can see
RTE_ETH_DEV_DETACHABLE was removed in 17.11. Does it
change something in above description?

>> I agree this is the problem.
>> In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
>>
>> but the generic code is faulty.
>>
>> And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.

Yes, as I understand RTE_ETH_DEV_CLOSE_REMOVE,
RTE_ETH_DEV_BONDED_SLAVE, RTE_ETH_DEV_REPRESENTOR and
RTE_ETH_DEV_NOLIVE_MAC_ADDR may be lost because of
reinit (if not restored in other branches). Bad anyway.

>>> Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
>>> enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
>>> function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
>>>
>>> in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
>>> another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
>>> shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
>>> I think the second way is simple and lower risk.
>>
>> Yes these are the two options.
>>
>> I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
>> BUT my concern was adding decision making to simple/leaf function and make it
>> harder to debug/use, instead of giving what primary/secondary process should
>> call decision in higher level.
>>
>> But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
>> secondary process, like mlx4 or szedata2, and most probably this is not their
>> intention.
>> And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
>> function may have side affect of 'eth_dev->intr_handle' not set in secondary.
>>
>> With above considerations I am OK to your proposal to cover all cases, Thomas,
>> Andrew, any concern?

I would put if condition in rte_eth_copy_pci_info().
It is the function which writes shared space from
secondary process when it should not be done and it
should be fixed there.

> Do you mean drivers need to be fixed?

I'm not sure that I fully understand it. Since copy function
cares about intr_handle copying I'm afraid that it is not
100% correct to skip it in secondary process completely as
many drivers do right now. Basically it makes eth_dev structure
in secondary process inconsistent. However, it looks like
most of these drivers simply obtain handle from pci_dev
directly and it explains why they are not affected.
There are exceptions which are potentially bugs, e.g.
drivers/net/ice/ice_ethdev.c: ice_interrupt_handler at the end.

I think that it would be better if intr_handle is always
correct in eth_dev (both primary and secondary cases) and
drivers use it instead of the same from pci_dev.

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Ferruh Yigit]

On 1/16/2020 7:43 AM, Andrew Rybchenko wrote:
> On 1/15/20 11:43 PM, Thomas Monjalon wrote:
>> 15/01/2020 19:35, Ferruh Yigit:
>>> On 1/15/2020 6:49 AM, 方统浩50450 wrote:
>>>> Hi Ferruh, thanks for your message.
>>>>
>>>>
>>>> We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
>>>> support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
>>>> secondary process will change the shared memory when initializing.Secondary process calls
>>>> "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
>>>> (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
>>>> Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
>>>> is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
>>>> the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
>>>> detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
>>>> the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
>>>> process.
> 
> Hold on, just for my understanding. As far as I can see
> RTE_ETH_DEV_DETACHABLE was removed in 17.11. Does it
> change something in above description?

Overall secondary overwrites primary values, I think we should fix it
independent from the flags involved.

> 
>>> I agree this is the problem.
>>> In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
>>>
>>> but the generic code is faulty.
>>>
>>> And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.
> 
> Yes, as I understand RTE_ETH_DEV_CLOSE_REMOVE,
> RTE_ETH_DEV_BONDED_SLAVE, RTE_ETH_DEV_REPRESENTOR and
> RTE_ETH_DEV_NOLIVE_MAC_ADDR may be lost because of
> reinit (if not restored in other branches). Bad anyway.
> 
>>>> Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
>>>> enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
>>>> function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
>>>>
>>>> in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
>>>> another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
>>>> shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
>>>> I think the second way is simple and lower risk.
>>>
>>> Yes these are the two options.
>>>
>>> I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
>>> BUT my concern was adding decision making to simple/leaf function and make it
>>> harder to debug/use, instead of giving what primary/secondary process should
>>> call decision in higher level.
>>>
>>> But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
>>> secondary process, like mlx4 or szedata2, and most probably this is not their
>>> intention.
>>> And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
>>> function may have side affect of 'eth_dev->intr_handle' not set in secondary.
>>>
>>> With above considerations I am OK to your proposal to cover all cases, Thomas,
>>> Andrew, any concern?
> 
> I would put if condition in rte_eth_copy_pci_info().
> It is the function which writes shared space from
> secondary process when it should not be done and it
> should be fixed there.

OK

> 
>> Do you mean drivers need to be fixed?
> 
> I'm not sure that I fully understand it. Since copy function
> cares about intr_handle copying I'm afraid that it is not
> 100% correct to skip it in secondary process completely as
> many drivers do right now. Basically it makes eth_dev structure
> in secondary process inconsistent. However, it looks like
> most of these drivers simply obtain handle from pci_dev
> directly and it explains why they are not affected.
> There are exceptions which are potentially bugs, e.g.
> drivers/net/ice/ice_ethdev.c: ice_interrupt_handler at the end.
> 
> I think that it would be better if intr_handle is always
> correct in eth_dev (both primary and secondary cases) and
> drivers use it instead of the same from pci_dev.
> 

OK

So this suggest going on with Fang's patch. I only requested an additional note
in function comment related to this secondary check.

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[方统浩50450]

>@Fang, only can you please make a new version to update the
>'rte_eth_copy_pci_info' function comment to document shared data is not updated
>for the secondary process?
>So this suggest going on with Fang's patch. I only requested an additional note
>in function comment related to this secondary check.
@Ferruh Yigit
Should I update a new version patch of "rte_eth_copy_pci_info" function and explain
wthether the regular functioning of secondary process is affected or not?
I cant figure out what you need me to do.




发件人：Ferruh Yigit <ferruh.yigit@intel.com>
发送日期：2020-01-16 17:04:09
收件人：Andrew Rybchenko <arybchenko@solarflare.com>,Thomas Monjalon <thomas@monjalon.net>
抄送人："方统浩50450" <fangtonghao@sangfor.com.cn>,dev@dpdk.org,stable@dpdk.org,jia.guo@intel.com,cunming.liang@intel.com,qi.z.zhang@intel.com,jungle845943968@outlook.com,Jerin Jacob Kollanukkaran <jerinj@marvell.com>
主题：Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory>On 1/16/2020 7:43 AM, Andrew Rybchenko wrote:
>> On 1/15/20 11:43 PM, Thomas Monjalon wrote:
>>> 15/01/2020 19:35, Ferruh Yigit:
>>>> On 1/15/2020 6:49 AM, 方统浩50450 wrote:
>>>>> Hi Ferruh, thanks for your message.
>>>>>
>>>>>
>>>>> We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
>>>>> support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
>>>>> secondary process will change the shared memory when initializing.Secondary process calls
>>>>> "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
>>>>> (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
>>>>> Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
>>>>> is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
>>>>> the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
>>>>> detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
>>>>> the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
>>>>> process.
>> 
>> Hold on, just for my understanding. As far as I can see
>> RTE_ETH_DEV_DETACHABLE was removed in 17.11. Does it
>> change something in above description?
>
>Overall secondary overwrites primary values, I think we should fix it
>independent from the flags involved.
>
>> 
>>>> I agree this is the problem.
>>>> In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
>>>>
>>>> but the generic code is faulty.
>>>>
>>>> And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.
>> 
>> Yes, as I understand RTE_ETH_DEV_CLOSE_REMOVE,
>> RTE_ETH_DEV_BONDED_SLAVE, RTE_ETH_DEV_REPRESENTOR and
>> RTE_ETH_DEV_NOLIVE_MAC_ADDR may be lost because of
>> reinit (if not restored in other branches). Bad anyway.
>> 
>>>>> Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
>>>>> enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
>>>>> function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
>>>>>
>>>>> in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
>>>>> another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
>>>>> shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
>>>>> I think the second way is simple and lower risk.
>>>>
>>>> Yes these are the two options.
>>>>
>>>> I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
>>>> BUT my concern was adding decision making to simple/leaf function and make it
>>>> harder to debug/use, instead of giving what primary/secondary process should
>>>> call decision in higher level.
>>>>
>>>> But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
>>>> secondary process, like mlx4 or szedata2, and most probably this is not their
>>>> intention.
>>>> And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
>>>> function may have side affect of 'eth_dev->intr_handle' not set in secondary.
>>>>
>>>> With above considerations I am OK to your proposal to cover all cases, Thomas,
>>>> Andrew, any concern?
>> 
>> I would put if condition in rte_eth_copy_pci_info().
>> It is the function which writes shared space from
>> secondary process when it should not be done and it
>> should be fixed there.
>
>OK
>
>> 
>>> Do you mean drivers need to be fixed?
>> 
>> I'm not sure that I fully understand it. Since copy function
>> cares about intr_handle copying I'm afraid that it is not
>> 100% correct to skip it in secondary process completely as
>> many drivers do right now. Basically it makes eth_dev structure
>> in secondary process inconsistent. However, it looks like
>> most of these drivers simply obtain handle from pci_dev
>> directly and it explains why they are not affected.
>> There are exceptions which are potentially bugs, e.g.
>> drivers/net/ice/ice_ethdev.c: ice_interrupt_handler at the end.
>> 
>> I think that it would be better if intr_handle is always
>> correct in eth_dev (both primary and secondary cases) and
>> drivers use it instead of the same from pci_dev.
>> 
>
>OK
>
>So this suggest going on with Fang's patch. I only requested an additional note
>in function comment related to this secondary check.

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Ferruh Yigit]

On 1/16/2020 11:35 AM, 方统浩50450 wrote:
> 
> 
>>@Fang, only can you please make a new version to update the
>>'rte_eth_copy_pci_info' function comment to document shared data is not updated
>>for the secondary process?
> 
>>So this suggest going on with Fang's patch. I only requested an additional note
>>in function comment related to this secondary check.
> 
> @Ferruh Yigit
> Should I update a new version patch of "rte_eth_copy_pci_info" function and explain
> wthether the regular functioning of secondary process is affected or not?
> I cant figure out what you need me to do.

Hi Fang,

Yes can you please send a new version of your patch.
In new version, additionally update the 'rte_eth_copy_pci_info()' function
comment to document that function updates 'eth_dev->data' only for primary process.

Thanks,
ferruh

> 
> 
> 发件人：Ferruh Yigit <ferruh.yigit@intel.com>
> 发送日期：2020-01-16 17:04:09
> 收件人：Andrew Rybchenko <arybchenko@solarflare.com>,Thomas Monjalon <thomas@monjalon.net>
> 抄送人："方统浩50450" <fangtonghao@sangfor.com.cn>,dev@dpdk.org,stable@dpdk.org,jia.guo@intel.com,cunming.liang@intel.com,qi.z.zhang@intel.com,jungle845943968@outlook.com,Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> 主题：Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory>On 1/16/2020 7:43 AM, Andrew Rybchenko wrote:
>>> On 1/15/20 11:43 PM, Thomas Monjalon wrote:
>>>> 15/01/2020 19:35, Ferruh Yigit:
>>>>> On 1/15/2020 6:49 AM, 方统浩50450 wrote:
>>>>>> Hi Ferruh, thanks for your message.
>>>>>>
>>>>>>
>>>>>> We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
>>>>>> support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
>>>>>> secondary process will change the shared memory when initializing.Secondary process calls
>>>>>> "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
>>>>>> (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
>>>>>> Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
>>>>>> is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
>>>>>> the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
>>>>>> detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
>>>>>> the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
>>>>>> process.
>>> 
>>> Hold on, just for my understanding. As far as I can see
>>> RTE_ETH_DEV_DETACHABLE was removed in 17.11. Does it
>>> change something in above description?
>>
>>Overall secondary overwrites primary values, I think we should fix it
>>independent from the flags involved.
>>
>>> 
>>>>> I agree this is the problem.
>>>>> In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
>>>>>
>>>>> but the generic code is faulty.
>>>>>
>>>>> And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.
>>> 
>>> Yes, as I understand RTE_ETH_DEV_CLOSE_REMOVE,
>>> RTE_ETH_DEV_BONDED_SLAVE, RTE_ETH_DEV_REPRESENTOR and
>>> RTE_ETH_DEV_NOLIVE_MAC_ADDR may be lost because of
>>> reinit (if not restored in other branches). Bad anyway.
>>> 
>>>>>> Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
>>>>>> enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
>>>>>> function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
>>>>>>
>>>>>> in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
>>>>>> another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
>>>>>> shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
>>>>>> I think the second way is simple and lower risk.
>>>>>
>>>>> Yes these are the two options.
>>>>>
>>>>> I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
>>>>> BUT my concern was adding decision making to simple/leaf function and make it
>>>>> harder to debug/use, instead of giving what primary/secondary process should
>>>>> call decision in higher level.
>>>>>
>>>>> But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
>>>>> secondary process, like mlx4 or szedata2, and most probably this is not their
>>>>> intention.
>>>>> And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
>>>>> function may have side affect of 'eth_dev->intr_handle' not set in secondary.
>>>>>
>>>>> With above considerations I am OK to your proposal to cover all cases, Thomas,
>>>>> Andrew, any concern?
>>> 
>>> I would put if condition in rte_eth_copy_pci_info().
>>> It is the function which writes shared space from
>>> secondary process when it should not be done and it
>>> should be fixed there.
>>
>>OK
>>
>>> 
>>>> Do you mean drivers need to be fixed?
>>> 
>>> I'm not sure that I fully understand it. Since copy function
>>> cares about intr_handle copying I'm afraid that it is not
>>> 100% correct to skip it in secondary process completely as
>>> many drivers do right now. Basically it makes eth_dev structure
>>> in secondary process inconsistent. However, it looks like
>>> most of these drivers simply obtain handle from pci_dev
>>> directly and it explains why they are not affected.
>>> There are exceptions which are potentially bugs, e.g.
>>> drivers/net/ice/ice_ethdev.c: ice_interrupt_handler at the end.
>>> 
>>> I think that it would be better if intr_handle is always
>>> correct in eth_dev (both primary and secondary cases) and
>>> drivers use it instead of the same from pci_dev.
>>> 
>>
>>OK
>>
>>So this suggest going on with Fang's patch. I only requested an additional note
>>in function comment related to this secondary check.
> 
> 

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[方统浩50450]

ok, I send a new version of my patch and rewrite commit log again.
you can check my patch in https://patches.dpdk.org/patch/64819/






发件人：Ferruh Yigit <ferruh.yigit@intel.com>
发送日期：2020-01-16 20:18:18
收件人："方统浩50450" <fangtonghao@sangfor.com.cn>
抄送人：Andrew Rybchenko <arybchenko@solarflare.com>,Thomas Monjalon <thomas@monjalon.net>,dev@dpdk.org,stable@dpdk.org,jia.guo@intel.com,cunming.liang@intel.com,qi.z.zhang@intel.com,jungle845943968@outlook.com,Jerin Jacob Kollanukkaran <jerinj@marvell.com>
主题：Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory>On 1/16/2020 11:35 AM, 方统浩50450 wrote:
>> 
>> 
>>>@Fang, only can you please make a new version to update the
>>>'rte_eth_copy_pci_info' function comment to document shared data is not updated
>>>for the secondary process?
>> 
>>>So this suggest going on with Fang's patch. I only requested an additional note
>>>in function comment related to this secondary check.
>> 
>> @Ferruh Yigit
>> Should I update a new version patch of "rte_eth_copy_pci_info" function and explain
>> wthether the regular functioning of secondary process is affected or not?
>> I cant figure out what you need me to do.
>
>Hi Fang,
>
>Yes can you please send a new version of your patch.
>In new version, additionally update the 'rte_eth_copy_pci_info()' function
>comment to document that function updates 'eth_dev->data' only for primary process.
>
>Thanks,
>ferruh
>
>> 
>> 
>> 发件人：Ferruh Yigit <ferruh.yigit@intel.com>
>> 发送日期：2020-01-16 17:04:09
>> 收件人：Andrew Rybchenko <arybchenko@solarflare.com>,Thomas Monjalon <thomas@monjalon.net>
>> 抄送人："方统浩50450" <fangtonghao@sangfor.com.cn>,dev@dpdk.org,stable@dpdk.org,jia.guo@intel.com,cunming.liang@intel.com,qi.z.zhang@intel.com,jungle845943968@outlook.com,Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>> 主题：Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory>On 1/16/2020 7:43 AM, Andrew Rybchenko wrote:
>>>> On 1/15/20 11:43 PM, Thomas Monjalon wrote:
>>>>> 15/01/2020 19:35, Ferruh Yigit:
>>>>>> On 1/15/2020 6:49 AM, 方统浩50450 wrote:
>>>>>>> Hi Ferruh, thanks for your message.
>>>>>>>
>>>>>>>
>>>>>>> We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
>>>>>>> support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
>>>>>>> secondary process will change the shared memory when initializing.Secondary process calls
>>>>>>> "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
>>>>>>> (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
>>>>>>> Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
>>>>>>> is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
>>>>>>> the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
>>>>>>> detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
>>>>>>> the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
>>>>>>> process.
>>>> 
>>>> Hold on, just for my understanding. As far as I can see
>>>> RTE_ETH_DEV_DETACHABLE was removed in 17.11. Does it
>>>> change something in above description?
>>>
>>>Overall secondary overwrites primary values, I think we should fix it
>>>independent from the flags involved.
>>>
>>>> 
>>>>>> I agree this is the problem.
>>>>>> In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
>>>>>>
>>>>>> but the generic code is faulty.
>>>>>>
>>>>>> And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.
>>>> 
>>>> Yes, as I understand RTE_ETH_DEV_CLOSE_REMOVE,
>>>> RTE_ETH_DEV_BONDED_SLAVE, RTE_ETH_DEV_REPRESENTOR and
>>>> RTE_ETH_DEV_NOLIVE_MAC_ADDR may be lost because of
>>>> reinit (if not restored in other branches). Bad anyway.
>>>> 
>>>>>>> Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
>>>>>>> enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
>>>>>>> function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
>>>>>>>
>>>>>>> in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
>>>>>>> another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
>>>>>>> shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
>>>>>>> I think the second way is simple and lower risk.
>>>>>>
>>>>>> Yes these are the two options.
>>>>>>
>>>>>> I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
>>>>>> BUT my concern was adding decision making to simple/leaf function and make it
>>>>>> harder to debug/use, instead of giving what primary/secondary process should
>>>>>> call decision in higher level.
>>>>>>
>>>>>> But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
>>>>>> secondary process, like mlx4 or szedata2, and most probably this is not their
>>>>>> intention.
>>>>>> And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
>>>>>> function may have side affect of 'eth_dev->intr_handle' not set in secondary.
>>>>>>
>>>>>> With above considerations I am OK to your proposal to cover all cases, Thomas,
>>>>>> Andrew, any concern?
>>>> 
>>>> I would put if condition in rte_eth_copy_pci_info().
>>>> It is the function which writes shared space from
>>>> secondary process when it should not be done and it
>>>> should be fixed there.
>>>
>>>OK
>>>
>>>> 
>>>>> Do you mean drivers need to be fixed?
>>>> 
>>>> I'm not sure that I fully understand it. Since copy function
>>>> cares about intr_handle copying I'm afraid that it is not
>>>> 100% correct to skip it in secondary process completely as
>>>> many drivers do right now. Basically it makes eth_dev structure
>>>> in secondary process inconsistent. However, it looks like
>>>> most of these drivers simply obtain handle from pci_dev
>>>> directly and it explains why they are not affected.
>>>> There are exceptions which are potentially bugs, e.g.
>>>> drivers/net/ice/ice_ethdev.c: ice_interrupt_handler at the end.
>>>> 
>>>> I think that it would be better if intr_handle is always
>>>> correct in eth_dev (both primary and secondary cases) and
>>>> drivers use it instead of the same from pci_dev.
>>>> 
>>>
>>>OK
>>>
>>>So this suggest going on with Fang's patch. I only requested an additional note
>>>in function comment related to this secondary check.
>> 
>> 
>

Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory

[Ferruh Yigit]

On 1/15/2020 8:43 PM, Thomas Monjalon wrote:
> 15/01/2020 19:35, Ferruh Yigit:
>> On 1/15/2020 6:49 AM, 方统浩50450 wrote:
>>> Hi Ferruh, thanks for your message.
>>>
>>>
>>> We developed a ethtool-dpdk which is secondary process based dpdk 17.08 version. Our device
>>> support hotplug detach, but hotplug deatch is failed when we use ethtool-dpdk.We found the
>>> secondary process will change the shared memory when initializing.Secondary process calls
>>> "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" function.
>>> (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_copy_pci_info)
>>> Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.In our platform, this value
>>> is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but after reset
>>> the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).So, our device hotplug
>>> detach is failed.I found the similar problem in other dpdk version, include dpdk 19.11.Even though
>>> the deivce hotplug detach is discarded,but i think the shared memory changed is unexpected by primary
>>> process.
>>
>> I agree this is the problem.
>> In the driver code, 'rte_eth_copy_pci_info' is called only by primary process,
>> but the generic code is faulty.
>>
>> And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same problem.
>>
>>> Our driver is ixgbe, i think this problem has a little relationship with driver, Secondary process
>>> enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agree your opinion, the helper
>>> function should simple on what it does.I have two ways to fix this problem, one is add an if-statement
>>>
>>> in "rte_eth_dev_pci_allocate" function to forbid secondary process enters "rte_eth_copy_pci_info" function,
>>> another way is add an if-statement in "rte_eth_copy_pci_info" function to forbid secondary process change
>>> shared memory.And First way need to ensure the "rte_eth_copy_pci_info" function won't be called anywhere else.
>>> I think the second way is simple and lower risk.
>>
>> Yes these are the two options.
>>
>> I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and safer.
>> BUT my concern was adding decision making to simple/leaf function and make it
>> harder to debug/use, instead of giving what primary/secondary process should
>> call decision in higher level.
>>
>> But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' on
>> secondary process, like mlx4 or szedata2, and most probably this is not their
>> intention.
>> And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling this
>> function may have side affect of 'eth_dev->intr_handle' not set in secondary.
>>
>> With above considerations I am OK to your proposal to cover all cases, Thomas,
>> Andrew, any concern?
> 
> Do you mean drivers need to be fixed?
> 

either it or 'rte_eth_copy_pci_info'.

Right now 'rte_eth_copy_pci_info' updates the shared memory, calling it in
secondary overwrites the memory set by primary.

Options Fang mentioned:
1) Don't call 'rte_eth_copy_pci_info' from secondary process path, this requires
fixing 'rte_eth_dev_pci_allocate', 'eth_dev_pci_specific_init' and possibly some
drivers.

2) Add a check inside the 'rte_eth_copy_pci_info' to prevent updating shared
memory if it is secondary process.

Fang's patch does (2), and I am OK with it as well after latest findings.

[dpdk-dev] [PATCH v3] Fixes: ethdev: secondary process change shared memory

[Fang TongHao]

Fixes the secondary process changed shared memory
in "rte_eth_copy_pci_info" function.In that function
only primary can update the value of "eth_dev->data"
which shared by primary and secondary.

Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
---
 lib/librte_ethdev/rte_ethdev_pci.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
index ccdbb46ec..e7dae0545 100644
--- a/lib/librte_ethdev/rte_ethdev_pci.h
+++ b/lib/librte_ethdev/rte_ethdev_pci.h
@@ -60,14 +60,16 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev,
 
 	eth_dev->intr_handle = &pci_dev->intr_handle;
 
-	eth_dev->data->dev_flags = 0;
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
-		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
-		eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
-
-	eth_dev->data->kdrv = pci_dev->kdrv;
-	eth_dev->data->numa_node = pci_dev->device.numa_node;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev->data->dev_flags = 0;
+		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
+			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC;
+		if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_RMV)
+			eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_RMV;
+
+		eth_dev->data->kdrv = pci_dev->kdrv;
+		eth_dev->data->numa_node = pci_dev->device.numa_node;
+	}
 }
 
 static inline int
-- 
2.24.1.windows.2

Re: [dpdk-dev] [PATCH v3] Fixes: ethdev: secondary process change shared memory

[Andrew Rybchenko]

On 1/17/20 5:08 AM, Fang TongHao wrote:

Summary does not comply with [1].

> Fixes the secondary process changed shared memory
> in "rte_eth_copy_pci_info" function.In that function
> only primary can update the value of "eth_dev->data"
> which shared by primary and secondary.

Consider:
Avoid overwriting device flags and other information in device
data stored in shared memory when a secondary process
probes PCI device.

Fixes tag and CC to stable is required in accordance with [2].

> Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>

[1]
https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-subject-line
[2]
https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body

Re: [dpdk-dev] [PATCH v3] Fixes: ethdev: secondary process change shared memory

[Ferruh Yigit]

On 1/17/2020 8:33 AM, Andrew Rybchenko wrote:
> On 1/17/20 5:08 AM, Fang TongHao wrote:
> 
> Summary does not comply with [1].
> 
>> Fixes the secondary process changed shared memory
>> in "rte_eth_copy_pci_info" function.In that function
>> only primary can update the value of "eth_dev->data"
>> which shared by primary and secondary.
> 
> Consider:
> Avoid overwriting device flags and other information in device
> data stored in shared memory when a secondary process
> probes PCI device.
> 
> Fixes tag and CC to stable is required in accordance with [2].
> 
>> Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
> 
> [1]
> https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-subject-line
> [2]
> https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body
> 

For sake of having the patch in -rc1, I have updated as suggested while merging,
commit log become as following:

    ethdev: fix secondary process memory overwrite

    Avoid overwriting device flags and other information in device
    data stored in shared memory when a secondary process
    probes PCI device.

    Fixes: 494adb7f63f2 ("ethdev: add device fields from PCI layer")
    Cc: stable@dpdk.org

    Signed-off-by: Fang TongHao <fangtonghao@sangfor.com.cn>
    Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
    Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>


And I have added a not the the function comment as following:
   * Copy pci device info to the Ethernet device data.
 + * Shared memory (eth_dev->data) only updated by primary process, so it is safe
 + * to call this function from both primary and secondary processes.
  *


Applied to dpdk-next-net/master, thanks.