[ovs-dev] [PATCH v2 1/3] netdev-dpdk: Fix double attaching of virtual devices.

Ilya Maximets i.maximets at samsung.com
Thu May 18 11:59:53 UTC 2017


On 17.05.2017 18:01, Darrell Ball wrote:
> 
> 
> On 5/17/17, 4:16 AM, "Ilya Maximets" <i.maximets at samsung.com> wrote:
> 
>     On 17.05.2017 08:53, Ilya Maximets wrote:
>     > Hi Darrell,
>     > 
>     > Good catch. Thanks. One comment inline.
>     > 
>     > Best regards, Ilya Maximets.
>     > 
>     > On 16.05.2017 20:14, Darrell Ball wrote:
>     >> I applied the following incremental to fix a missing free on error, but otherwise is fine 
>     >> Can you fold in the incremental.
>     >>
>     >>
>     >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     >> index 56b9d25..25d9c9b 100644
>     >> --- a/lib/netdev-dpdk.c
>     >> +++ b/lib/netdev-dpdk.c
>     >> @@ -1128,7 +1128,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >>          } else {
>     >>              /* Attach unsuccessful */
>     >>              VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK", devargs);
>     >> -            return -1;
>     >> +            new_port_id = UINT8_MAX;
>     > 
>     > 'rte_eth_dev_get_port_by_name' and 'rte_eth_dev_attach' set a valid
>     > 'new_port_id' only on success. So, we don't need to reinitialize it
>     > here. Just removing of return should be enough.
>     
>     I just found that this change also was mistakenly moved to the third patch.
>     Oh.. Kind of bad luck with splitting of this patches.
>     Also, I think, I'll keep this reinitialization just to make code more clear.
> 
> In your previous response, you were arguing that removing the return was good
> enough. Now, you are saying the re-initialization is ok since you had something 
> similar in your 3rd patch – hmm, fine.

There is one more reason. 'rte_eth_dev_get_port_by_name' actually changes
the new_port_id even on failure. It sets new_port_id to RTE_MAX_ETHPORTS.
It's harmless, but looks logically wrong. This is actually undocumented
behaviour and I decided to re-initialize just to avoid any possible issue
in the future.

In general, I think, that 'rte_eth_dev_get_port_by_name' shouldn't touch
the new_port_id in case of failure. I'll send corresponding patch to DPDK.
    
>     >>          }
>     >>      }
>     >>
>     >>
>     >> On 5/12/17, 8:10 AM, "Ilya Maximets" <i.maximets at samsung.com> wrote:
>     >>
>     >>     'devargs' for virtual devices contains not only name but
>     >>     also a list of arguments like this:
>     >>     
>     >>     	'net_pcap0,rx_pcap=file_rx.pcap,tx_pcap=file_tx.pcap'
>     >>     	or
>     >>     	'eth_af_packet0,iface=eth0'
>     >>     
>     >>     We must cut off the arguments from this string before calling
>     >>     'rte_eth_dev_get_port_by_name()' to avoid double attaching of
>     >>     the same device.
>     >>     
>     >>     CC: Ciara Loftus <ciara.loftus at intel.com>
>     >>     Fixes: 69876ed78611 ("netdev-dpdk: Add support for virtual DPDK PMDs (vdevs)")
>     >>     Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>     >>     ---
>     >>     
>     >>      lib/netdev-dpdk.c | 5 ++++-
>     >>      1 file changed, 4 insertions(+), 1 deletion(-)
>     >>     
>     >>     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     >>     index 609b8da..56b9d25 100644
>     >>     --- a/lib/netdev-dpdk.c
>     >>     +++ b/lib/netdev-dpdk.c
>     >>     @@ -1114,10 +1114,12 @@ netdev_dpdk_lookup_by_port_id(int port_id)
>     >>      static int
>     >>      netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >>      {
>     >>     +    /* Get the name up to the first comma. */
>     >>     +    char *name = xmemdup0(devargs, strcspn(devargs, ","));
>     >>          uint8_t new_port_id = UINT8_MAX;
>     >>      
>     >>          if (!rte_eth_dev_count()
>     >>     -            || rte_eth_dev_get_port_by_name(devargs, &new_port_id)
>     >>     +            || rte_eth_dev_get_port_by_name(name, &new_port_id)
>     >>                  || !rte_eth_dev_is_valid_port(new_port_id)) {
>     >>              /* Device not found in DPDK, attempt to attach it */
>     >>              if (!rte_eth_dev_attach(devargs, &new_port_id)) {
>     >>     @@ -1130,6 +1132,7 @@ netdev_dpdk_process_devargs(const char *devargs, char **errp)
>     >>              }
>     >>          }
>     >>      
>     >>     +    free(name);
>     >>          return new_port_id;
>     >>      }
>     >>      
>     >>     -- 
>     >>     2.7.4
>     >>     
>     >>     
>     >>
>     
> 


More information about the dev mailing list