From patchwork Thu Dec 22 11:05:18 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Traynor X-Patchwork-Id: 708205 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3tkpbC1Ysmz9srZ for ; Thu, 22 Dec 2016 22:05:27 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 87F35B79; Thu, 22 Dec 2016 11:05:23 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id B4639B62 for ; Thu, 22 Dec 2016 11:05:22 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id EB153D5 for ; Thu, 22 Dec 2016 11:05:21 +0000 (UTC) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4D8DF9C0CB; Thu, 22 Dec 2016 11:05:21 +0000 (UTC) Received: from ktraynor.remote.csb (vpn1-5-24.ams2.redhat.com [10.36.5.24]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uBMB5JhH004989; Thu, 22 Dec 2016 06:05:19 -0500 To: Daniele Di Proietto References: <1481802856-4318-1-git-send-email-ciara.loftus@intel.com> <1481802856-4318-2-git-send-email-ciara.loftus@intel.com> <9eaf6bbc-abaa-e56b-5d4b-a06596707dad@redhat.com> <23e03654-fd08-6544-7814-371207d4e9eb@redhat.com> From: Kevin Traynor X-Enigmail-Draft-Status: N1110 Organization: Red Hat Message-ID: <86cca913-fb4d-0c58-66d0-3b7b58e2a75e@redhat.com> Date: Thu, 22 Dec 2016 11:05:18 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <23e03654-fd08-6544-7814-371207d4e9eb@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 22 Dec 2016 11:05:21 +0000 (UTC) X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: "dev@openvswitch.org" Subject: Re: [ovs-dev] [PATCH v3 2/3] netdev-dpdk: Arbitrary 'dpdk' port naming X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org On 12/22/2016 10:02 AM, Kevin Traynor wrote: > On 12/21/2016 07:35 PM, Daniele Di Proietto wrote: >> 2016-12-21 10:18 GMT-08:00 Kevin Traynor : >>> On 12/21/2016 03:02 AM, Daniele Di Proietto wrote: >>>> 2016-12-20 14:08 GMT-08:00 Kevin Traynor : >>>>> On 12/15/2016 11:54 AM, Ciara Loftus wrote: >>>>>> 'dpdk' ports no longer have naming restrictions. Now, instead of >>>>>> specifying the dpdk port ID as part of the name, the PCI address of the >>>>>> device must be specified via the 'dpdk-devargs' option. eg. >>>>>> >>>>>> ovs-vsctl add-port br0 my-port >>>>>> ovs-vsctl set Interface my-port type=dpdk >>>>>> ovs-vsctl set Interface my-port options:dpdk-devargs=0000:06:00.3 >>>>> >>>>> I wouldn't encourage people to split up commands like above as they'll >>>>> see errors and warnings. >>>> >>>> Good point >>>> >>>>> >>>>> If you use the old command (which people surely will), it's not >>>>> intuitive that it's now still a valid cmd but incomplete for setting up >>>>> the port: >>>>> >>>>> []# ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk >>>>> 2016-12-20T19:53:54Z|00051|netdev|WARN|dpdk0: could not set >>>>> configuration (Invalid argument) >>>>> ovs-vsctl: Error detected while setting up 'dpdk0'. See ovs-vswitchd >>>>> log for details. >>>>> >>>>> It would be nice if this was just a warning and more informative - this >>>>> could be an incremental change also. >>>> >>>> You're right, vsctl errors are pretty obscure in this case. I think a first >>>> step is to improve what ovs-vsctl reports to the user. I sent a patch here: >>>> >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2016-December/326542.html >>>> >>>> The second step would be to allow netdev_dpdk_set_config() to return an error >>>> string, that can be printed by ovs-vsctl. I'm fine with doing that later. >>>> What do you guys think? >>> >>> sounds like a good plan to me. >>> >>> I've done some testing with this patch today and I can't seem to get >>> hotplug working after applying 2/3. It works with 1/3 but I'm seeing >>> hangs with the new scheme in 2/3 and a dpdk seg fault with 3/3. I think >>> maybe my dpdk build is bad or my steps are wrong but it would be good if >>> someone else could test too. >>> >>> - dpdk-devbind.py -u 0000:01:00.0 0000:01:00.1 >>> - start vswitchd and add bridge >>> - dpdk-devbind.py -b igb_uio 0000:01:00.0 0000:01:00.1 >>> - ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk >>> options:dpdk-devargs=0000:01:00.0 >>> >> >> I think there's a bug in DPDK 16.11 that should be fixed by this commit: >> >> f9ae888b1e19("ethdev: fix port lookup if none"). >> >> Does it work if you include the fix in your DPDK build? > > yes - that was the issue I was hitting for the seg fault, thanks. The > hang is fixed too, I think my igb_uio was out of date. > > It is a reasonable that someone might try to mistakenly add a port when > there are no devices in dpdk but the dpdk fix won't be available until a > 16.11.1, so we need the equivalent check in OVS. I tested attach/detach > when no device with/without this incremental and it works now. pah...I didn't test when there was a single device bound after vswitchd starts and of course it breaks, so it should be: goto error; } > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 75559fe..bedff86 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1097,6 +1097,10 @@ netdev_dpdk_process_devargs(const char *devargs) > { > uint8_t new_port_id = UINT8_MAX; > > + if (!rte_eth_dev_count()) { > + goto out; > + } > + > if (rte_eth_dev_get_port_by_name(devargs, &new_port_id) > || !rte_eth_dev_is_valid_port(new_port_id)) { > /* Device not found in DPDK, attempt to attach it */ > @@ -1109,6 +1113,7 @@ netdev_dpdk_process_devargs(const char *devargs) > } > } > > +out: > return new_port_id; > } > > @@ -2397,7 +2402,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int > argc OVS_UNUSED, > > ovs_mutex_lock(&dpdk_mutex); > > - if (rte_eth_dev_get_port_by_name(argv[1], &port_id)) { > + if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1], > &port_id)) { > response = xasprintf("Device '%s' not found in DPDK", argv[1]); > goto error; > } > > >> >>> >>>> >>>>> >>>>>> >>>>>> The user must no longer hotplug attach DPDK ports by issuing the >>>>>> specific ovs-appctl netdev-dpdk/attach command. The hotplug is now >>>>>> automatically invoked when a valid PCI address is set in the >>>>>> dpdk-devargs. The format for ovs-appctl netdev-dpdk/detach command >>>>>> has changed in that the user now must specify the relevant PCI address >>>>>> as input instead of the port name. >>>>>> >>>>>> Signed-off-by: Ciara Loftus >>>>>> Signed-off-by: Daniele Di Proietto >>>>>> Co-authored-by: Daniele Di Proietto >>>> >>>> I think netdev_dpdk_set_config() should compare new_devargs with dev->devargs >>>> and avoid calling netdev_dpdk_process_devargs() if they're equal and >>>> if the port_id >>>> is valid. I've noticed that rte_eth_dev_get_port_by_name() sometimes >>>> doesn't find >>>> an existing device and I think comparing the strings will fix the problem. >>>> >>>> Also, could you add a log message if rte_eth_dev_attach fails? >>>> >>>> I've been playing with this for a while and I guess I'm fine with the >>>> rest of the series. >>>> >>>> Thanks, >>>> >>>> Daniele >>>> >>>>>> --- >>>>>> Changelog: >>>>>> * Keep port-detach appctl function - use PCI as input arg >>>>>> * Add requires_mutex to devargs processing functions >>>>>> * use reconfigure infrastructure for devargs changes >>>>>> * process devargs even if valid portid ie. device already configured >>>>>> * report err if dpdk-devargs is not specified >>>>>> * Add Daniele's incremental patch & Sign-off + Co-author tags >>>>>> * Update details of detach command to reflect PCI is needed instead of >>>>>> port name >>>>>> * Update NEWS to mention that the new naming scheme is not backwards >>>>>> compatible >>>>>> * Use existing DPDk functions to get port ID from PCI address/devname >>>>>> * Merged process_devargs and process_pdevargs functions >>>>>> * Removed unnecessary requires_mutex from devargs processing fn >>>>>> * Fix case where port is re-attached after detach >>>>>> * Add note to documentation that devices won't work until devargs set. >>>>>> >>>>>> Documentation/intro/install/dpdk-advanced.rst | 5 +- >>>>>> Documentation/intro/install/dpdk.rst | 15 ++- >>>>>> NEWS | 5 + >>>>>> lib/netdev-dpdk.c | 169 +++++++++++++++++--------- >>>>>> vswitchd/vswitch.xml | 8 ++ >>>>>> 5 files changed, 138 insertions(+), 64 deletions(-) >>> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 75559fe..11c007d 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1097,8 +1097,9 @@ netdev_dpdk_process_devargs(const char *devargs) { uint8_t new_port_id = UINT8_MAX; - if (rte_eth_dev_get_port_by_name(devargs, &new_port_id) - || !rte_eth_dev_is_valid_port(new_port_id)) { + if (!rte_eth_dev_count() + || rte_eth_dev_get_port_by_name(devargs, &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)) { /* Attach successful */ @@ -2397,7 +2398,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc OVS_UNUSED, ovs_mutex_lock(&dpdk_mutex); - if (rte_eth_dev_get_port_by_name(argv[1], &port_id)) { + if (!rte_eth_dev_count() || rte_eth_dev_get_port_by_name(argv[1], &port_id)) { response = xasprintf("Device '%s' not found in DPDK", argv[1]);