2016-12-13 11:30 GMT-08:00 Loftus, Ciara <[email protected]>: >> >> >> 2016-10-28 7:45 GMT-07:00 Ciara Loftus <[email protected]>: >> 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.
I think we can iterate through dpdk_list and check if that port_id is in use. To make it more efficient we could use a hash map instead of a list, but I'm not worried about that since the number of dpdk ports that we can have is rather limited anyway. > 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. I think this is fine as it is > >> 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 a lot for the new version! At this point I don't think we should rebase this on my series, I'd like to get this first and then I'll rebase my series. Daniele > > Thanks, > Ciara _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
