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

[方统浩50450 <fangtonghao@sangfor.com.cn>]

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