Re: [dpdk-dev] [PATCH v5 1/2] librte_ether: add protection against overwrite device data

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-10-07 12:23, Kerlin, MarcinX:
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, October 06, 2016 4:53 PM
> > To: Kerlin, MarcinX <marcinx.kerlin@intel.com>
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@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.