[dpdk-dev,1/2] lib/librte_ether: export secondary attach function

  Today eth_dev_attach_secondary is defined as static and can only be
called by pci drivers. However, the functionality is also required for
non-pci drivers - so the patch export the function.

Signed-off-by: Ami Sabo <amis@radware.com>
---
 lib/librte_ether/rte_ethdev.c          |  6 +++---
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++++++
 3 files changed, 23 insertions(+), 3 deletions(-)
  

Cc Thomas, the librte_ether maintainer.

On Sun, Feb 26, 2017 at 11:55:25AM +0200, Ami Sabo wrote:
>  /**
>   * @internal
> + * Attach to the ethdev already initialized by the primary
> + * process.
> + *
> + * @param       name    Ethernet device's name.
> +  @return

mailformed comment: missing *.

> + *   - Success: Slot in the rte_dev_devices array for attached
> + *        device.
> + *   - Error: Null pointer.
> + */
> +struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name);
> +
> +/**
> + * @internal
>   * Release the specified ethdev port.
>   *
>   * @param eth_dev
> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
> index c6c9d0d..d34c57a 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -154,3 +154,10 @@ DPDK_17.02 {
>  	rte_flow_validate;
>  
>  } DPDK_16.11;
> +
> +DPDK_17.05 {
> +        global:
> +
> +        rte_eth_dev_attach_secondary;

I saw whitespace chars. Like code, it should be TABs. Other than those
minor nits, it looks good to me.

Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu
  

The patchset fixes secondary process crash issue when it tries
to access virtio-user pmd (e.g. via rte_eth_rx_burst).

The crash happens because the secondary process calls, at
virtio_user_pmd_probe() to virtio_user_eth_dev_alloc()->
rte_eth_dev_allocate() instead of eth_dev_attach_secondary(), as it's
done from rte_eth_dev_pci_probe. Therefore, the device is not properly
initialized + the device data maybe overridden by the secondary
process.

The patchset contains 3 patches:
1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
allowed to call it.
2. Fix the actual bug by calling the function during virtio_user probe.
3. Code style fixes following Yuanhan Lio's comments.


Ami Sabo (3):
  lib/librte_ether: export secondary attach function
  net/virtio-user: fix multi-process issue
  lib/librte_ether: fix code style issues

 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 lib/librte_ether/rte_ethdev.c           |  6 +++---
 lib/librte_ether/rte_ethdev.h           | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map  |  7 +++++++
 4 files changed, 39 insertions(+), 13 deletions(-)

--
v2:
* Fix code style issues following Yuanhan Liu's review.
  

The patchset fixes secondary process crash issue when it tries
to access virtio-user pmd (e.g. via rte_eth_rx_burst).

The crash happens because in virtio_user probing,
eth_dev_attach_secondary is not being called, as it does from
rte_eth_dev_pci_probe. Therefore, the device is not properly
initialized.

The patchset contains 2 patches:
1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
allowed to call it.
2. Fix the actual bug by calling the function during virtio_user probe.

Ami Sabo (2):
  lib/librte_ether: export secondary attach function
  net/virtio-user: fix multi-process issue

 drivers/net/virtio/virtio_user_ethdev.c | 26 ++++++++++++++++----------
 lib/librte_ether/rte_ethdev.c           |  6 +++---
 lib/librte_ether/rte_ethdev.h           | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map  |  6 ++++++
 4 files changed, 38 insertions(+), 13 deletions(-)
  

2017-03-02 11:00, Ami Sabo:
> The patchset fixes secondary process crash issue when it tries
> to access virtio-user pmd (e.g. via rte_eth_rx_burst).
> 
> The crash happens because in virtio_user probing,
> eth_dev_attach_secondary is not being called, as it does from
> rte_eth_dev_pci_probe. Therefore, the device is not properly
> initialized.
> 
> The patchset contains 2 patches:
> 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
> allowed to call it.
> 2. Fix the actual bug by calling the function during virtio_user probe.

I do not understand why nobody complains for other virtual devices.
We should have the same issue with pcap, tap, ring, af_packet, etc.
Probably that other drivers are broken in secondary processes.
Or should we make a fix to handle every secondary vdev in
rte_eth_dev_allocate() ?
  

Ping

2017-03-08 12:40, Thomas Monjalon:
> 2017-03-02 11:00, Ami Sabo:
> > The patchset fixes secondary process crash issue when it tries
> > to access virtio-user pmd (e.g. via rte_eth_rx_burst).
> > 
> > The crash happens because in virtio_user probing,
> > eth_dev_attach_secondary is not being called, as it does from
> > rte_eth_dev_pci_probe. Therefore, the device is not properly
> > initialized.
> > 
> > The patchset contains 2 patches:
> > 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
> > allowed to call it.
> > 2. Fix the actual bug by calling the function during virtio_user probe.
> 
> I do not understand why nobody complains for other virtual devices.
> We should have the same issue with pcap, tap, ring, af_packet, etc.
> Probably that other drivers are broken in secondary processes.
> Or should we make a fix to handle every secondary vdev in
> rte_eth_dev_allocate() ?

Any opinion?
  

Hi Thomas,


On 4/7/2017 4:14 AM, Thomas Monjalon wrote:
> Ping
>
> 2017-03-08 12:40, Thomas Monjalon:
>> 2017-03-02 11:00, Ami Sabo:
>>> The patchset fixes secondary process crash issue when it tries
>>> to access virtio-user pmd (e.g. via rte_eth_rx_burst).
>>>
>>> The crash happens because in virtio_user probing,
>>> eth_dev_attach_secondary is not being called, as it does from
>>> rte_eth_dev_pci_probe. Therefore, the device is not properly
>>> initialized.
>>>
>>> The patchset contains 2 patches:
>>> 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
>>> allowed to call it.
>>> 2. Fix the actual bug by calling the function during virtio_user probe.
>> I do not understand why nobody complains for other virtual devices.
>> We should have the same issue with pcap, tap, ring, af_packet, etc.

Yes, none of vdev except ring supports primary/secondary mode.

>> Probably that other drivers are broken in secondary processes.
>> Or should we make a fix to handle every secondary vdev in
>> rte_eth_dev_allocate() ?

Agreed. We can change the rte_eth_dev_allocate() like this:
if (primary)
     allocate();
else
     lookup(name);

Besides, we need each vdev to handle its private. For example, pcap 
should share the selectable unix fd with secondary process; virtio-user 
should share callfds and kickfds; tap should share the fd pointing to 
/dev/tun.

Thanks,
Jianfeng
  

2017-03-02 11:00, Ami Sabo:
> The patchset fixes secondary process crash issue when it tries
> to access virtio-user pmd (e.g. via rte_eth_rx_burst).
> 
> The crash happens because in virtio_user probing,
> eth_dev_attach_secondary is not being called, as it does from
> rte_eth_dev_pci_probe. Therefore, the device is not properly
> initialized.
> 
> The patchset contains 2 patches:
> 1. Export rte_eth_dev_attach_secondary, so non-pci drivers will be
> allowed to call it.
> 2. Fix the actual bug by calling the function during virtio_user probe.
> 
> Ami Sabo (2):
>   lib/librte_ether: export secondary attach function
>   net/virtio-user: fix multi-process issue

Applied, thanks