[ovs-dev] [PATCH RFC v4 0/3] Port Hotplug, Arbitrary 'dpdk' Port Naming & Experimental vdev PMD Support

Loftus, Ciara ciara.loftus at intel.com
Tue Dec 13 19:30:00 UTC 2016


> 
> 
> 2016-10-28 7:45 GMT-07:00 Ciara Loftus <ciara.loftus at intel.com>:
> This RFC series consists of 3 patches.
> 1. Port Hotplug (Mauricio Vasquez) (v8)
> Previous: http://openvswitch.org/pipermail/dev/2016-July/075350.html
> 
> 2. Arbitrary Port Naming (Ciara Loftus) (v4)
> Previous: http://openvswitch.org/pipermail/dev/2016-July/075385.html
> 
> 3. Experimental vDev PMD (Ciara Loftus) (v1)
> Inspired by suggestions made during the v3 review:
> http://openvswitch.org/pipermail/dev/2016-July/075861.html
> 
> The version (4) is relative to the original Arbitrary port naming patch
> (see link #2).
> 
> Change log:
> 
> v4:
> - Rebase
> - Include Hotplug patch
> - Add vdev patch
> 
> v3:
> - remove global pci list
> - remove unnecessary parenthesis
> - remove return from void fn
> - print pci like dpdk
> - fix port ranges
> 
> v2:
> - Applied changes on top of hotplug patch and made the combination
>   compatible.
> 
> 
> Ciara Loftus (2):
>   netdev-dpdk: Arbitrary 'dpdk' port naming
>   netdev-dpdk: Add new 'dpdkvdev' experimental port type
> 
> Mauricio Vasquez (1):
>   netdev-dpdk: add hotplug support
> 
> Thanks for the series!
> I tested it and I was able to add ports using the pcap and af_packet pmd, I think
> that's extremely useful for testing.
> I have a few comments:
> As discussed in the summit I think it's safe to merge dpdk and dpdkvdev types.
> I'm not too worried about dpdkvdev being more 'experimental', I think we
> should try to design a consistent interface.
> 
> netdev_dpdk_process_vdevargs() and netdev_dpdk_process_pdevargs() need
> OVS_REQUIRES(dev->mutex) annotations.
> In the first patch, instead of using snprintf() with a fixed size buffer, how about
> using xasprintf()?
> Almost all the code introduced in the first patch is removed in the following
> one.  I guess we could keep the detach appctl, in case someone wants to bind a
> device back to the kernel stack without killing ovs-vswitchd (one could argue
> about also keeping the attach appctl, for consistency, I'm not sure about that)

I looked into keeping the detach. We'd have to change it to use the PCI address as input argument instead of the name since the name no longer tells us the port ID. We could retrieve the port ID associated with the PCI address using DPDK calls and subsequently call rte_eth_dev_detach(port_id). However I can't see a way to check if this port is being used by OVS or not. Before detach is called, the netdev struct and netdev_dpdk must == NULL so we can't query either for port_id information.
My question here is - do we want to be able to detach devices that aren't/haven't ever been used by an OVS port? This is the case in the most recent version of the patch. I can take it out in the following version depending on the consensus.

> netdev_dpdk_set_config() should always process dpdk-devargs even if
> rte_eth_dev_is_valid_port(dev->port_id) is true.  We have to adapt if the user
> changes the dpdk-devargs option in an Interface in the database.
> I think we could use reconfiguration to adapt to dpdk-devargs changes.  I believe
> that calling rte_eth_dev_stop() is not possible when traffic is flowing.
> 
> netdev_dpdk_set_config() should return an error if the dpdk-devargs is not in the
> options or if the value is invalid or if we fail to attach, so that the error can be
> reported to ovs-vsctl.
> Lastly, I sent a series that refactors dpdk port creation and postpones
> initialization to the first reconfiguration (until commit 6), maybe it can make this
> patch easier:
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2016-November/325141.html
> Thanks,
> Daniele


Thanks for the review Daniele. I've sent a new version today. I reworked to include your suggestions. I'm ok with them all, but just a small remark on one of them (see above). The current version isn't based on the series you mentioned above. If you think it would be useful I can do that for the next version, just let me know.

Thanks,
Ciara


More information about the dev mailing list