[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