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

[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. 

> 
> > --- 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.

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.

Regards,
Marcin