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

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

2016-10-06 13:57, Kerlin, MarcinX:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 
> > Hi Marcin,
> > 
> > 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.
> > 
> > I've just realized that you are trying to fix an useless code.
> > Why not just remove the array rte_eth_dev_data[] at first?
> 
> because pointer to rte_eth_dev_data in rte_eth_devices[] is 
> just to array rte_eth_dev_data[].
> 
> rte_ethdev.c:214 
> eth_dev->data = &rte_eth_dev_data[port_id];

This line indicates that the pointer data is to the struct rte_eth_dev_data
of the port_id, not the array of every devices.

> > We already have the array rte_eth_devices[] and there is a pointer to
> > rte_eth_dev_data in rte_eth_dev.
> 
> As you write above there is a pointer, but after run secondary testpmd this pointer
> will indicate data which hold from now data for secondary testpmd.
[...]
I think I've understood the bug.
I'm just saying you are fixing a weird design (rte_eth_dev_data[]).

> > Is it just a workaround to be able to lookup the rte_eth_dev_data memzone in
> > the secondary process?
> 
> No it is not workaround, it is protection against overwrite device data.
> I think that my cover letter good explain what is wrong. I did there
> short debug log.

I'm talking about the initial introduction of rte_eth_dev_data[]
which seems to be a workaround for multi-process without touching
rte_eth_devices[] allocated as a global variable (not a memzone).

> > So wouldn't it be saner to have rte_eth_devices[] in a memzone?
> 
> So you mean that move rte_eth_devices[] to memzone + remove rte_eth_dev_data[].

Yes

> What will indicate pointer inside rte_eth_dev  rte_eth_devices[]:
> (struct rte_eth_dev_data *data;  /**< Pointer to device data */)

After thinking more about it, I realize that rte_eth_devices cannot be
in a shared memzone because of its local pointers.

Sorry for the noise, I'll reconsider your patch.