2016-10-07 12:23, Kerlin, MarcinX: > Hi Thomas, > > > -----Original Message----- > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > Sent: Thursday, October 06, 2016 4:53 PM > > To: Kerlin, MarcinX <marcinx.kerlin at intel.com> > > Cc: dev at dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch at > > intel.com> > > Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrite > > device data > > > > 2016-09-30 16:00, Marcin Kerlin: > > > Added protection against overwrite device data in array > > > rte_eth_dev_data[] for the next secondary applications. Secondary > > > process appends in the first free place rather than at the beginning. > > > This behavior prevents overwriting devices data of primary process by > > secondary process. > > > > It would be good to state what is a secondary process. > > You are trying to extend its capabilities to be able to initialize devices. > > So primary and secondary processes are almost equivalent? > > What happens if we do not create any device in the primary? > > Answer from code review: "Cannot allocate memzone for ethernet port data\n" > > > > The secondary process is a hack to me. > > But it is fine to have such hack for debug or monitoring purpose. > > I would like to understand what are the other use cases? > > It's true, it is fine for debug or monitoring but If DPDK allow run secondary > app with > devices then it should be safe or completely not allowed. > > This bug has been observed while running secondary testpmd with virtual > devices. > > I will adapt to the decision of maintainers regards to design of secondary > process. > > > > > By the way, the code managing the shared data of a device should be at the > > EAL level in order to be used by other interfaces like crypto. > > > > > @@ -631,6 +692,8 @@ int > > > rte_eth_dev_detach(uint8_t port_id, char *name) { > > > struct rte_pci_addr addr; > > > + struct rte_eth_dev_data *eth_dev_data = NULL; > > > + char device[RTE_ETH_NAME_MAX_LEN]; > > > int ret = -1; > > > > > > if (name == NULL) { > > > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name) > > > if (rte_eth_dev_is_detachable(port_id)) > > > goto err; > > > > > > + /* get device name by port id */ > > > + if (rte_eth_dev_get_name_by_port(port_id, device)) > > > + goto err; > > > + > > > + /* look for an entry in the shared device data */ > > > + eth_dev_data = rte_eth_dev_get_dev_data_by_name(device); > > > + if (eth_dev_data == NULL) > > > + goto err; > > > > Why not getting eth_dev_data from rte_eth_devices[port_id].data ? > > because rte_eth_devices[port_id].data for some drivers (mainly virtual > devices) > is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other > drivers). > This causes that local eth_dev_data is clearing rather than shared in > memzone. > > Naming is unique so if device was added then there (shared memzone) has to > be.
Not sure to understand. Isn't it a bug to have local eth_dev_data? It means these devices are not shared with secondary process? > > > --- a/lib/librte_ether/rte_ethdev.h > > > +++ b/lib/librte_ether/rte_ethdev.h > > > /** > > > * @internal > > > + * Release device data kept in shared memory for all processes. > > > + * > > > + * @param port_id > > > + * The port identifier of the device to release device data. > > > + * @return > > > + * - 0 on success, negative on error > > > + */ > > > +int rte_eth_dev_release_dev_data(uint8_t port_id); > > > > Why this function? It is not used. > > You already have done the job in the detach function. > > This function is using in testpmd.c:1640, basic wrapper for clean up. Please explain the need for cleaning on testpmd exit. Is it cleaning every devices even those used by the primary process? I feel it is very weak to not clearly define which process owns a device. > Detach function is working only for detachable devices, release_dev_data() > no matter just clean up shared array before next run secondary e.g testpmd. Yes freeing device resources is for detachable devices.
