From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 472961BDDD for ; Wed, 27 Jun 2018 15:32:45 +0200 (CEST) Received: by mail-wm0-f66.google.com with SMTP id z6-v6so18144142wma.0 for ; Wed, 27 Jun 2018 06:32:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=kbdk0wJncCO7A7jZfbUmWhHzbQGavYQDU2GjPHl/PjU=; b=lTHFn6AULAQUo+peLT6ygtta4rIp5shvFZ1j3Hkb/AprooujagyZdINZ5pR9R8fFeu opeL0xPkGVQosrWXyKgV/2KXfbIZsPf/Ur1ZlVrkNOzVHxz/SDwyDIBhHj8dJeYSSXFP dM1ntvEh30kkNog9adyjWWwDLLIgAQZOaMzVyeY9Brh998nnPrQK/YzmL4+6dUjOx/l6 B4jJKm3CXxGyTKl0vTJOBlFH0swVJUNGmYN2w8tGE7u8ayhPO0LSD0wDFOZAmWONZvqp VbmIrn0fd054hwAhhFQKjnakRd3nqdEUNkHrTCN01icCp4NYmRWVUIhkD6ZaU8CKBWtr CCAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=kbdk0wJncCO7A7jZfbUmWhHzbQGavYQDU2GjPHl/PjU=; b=fITy985qhc9pdc6StXaUtEfQBy1TJO2m44TFNq8g4os/m/bZRDRlse6pODZD6nvxAf M24HvXo+EMwymxcUCqtRxeorDElK00zr2kZZnwuQchoxsKX0Z/b2Da5Dol6FO8t/iZrv o/o9hiUs+Kx6Dik9UPouWkX13T9kATBPlqbKbHf6sjYA/nvZLM0UseZFmgDkn0n9lJuv dliCtoKKx0d2cGpPgNvFcAVqEOUJ3fo8moMO8B83mWw6H+wP7RbqDFT8dYbo/Efk3y1K uK5sAlC8afHe+SX2id4kvm0lDxSPtzEvKCzna6nvK1gGxXSEjxv+lcbtcJTUpalItIz2 W5Uw== X-Gm-Message-State: APt69E2a5MJ3+neRrlZNOi4JQZgl8mUawGMzuruvNhSejdLkBHPX5owj EWbIFk33LV7jIgmVkv2ZfdL6bQ== X-Google-Smtp-Source: AAOMgpdCoKVgvtk8I0nW6rylLp6ku4FCKgdgPq/6m8dovhTB7/e+la87X8JhuTi80qax+6L57A2xcA== X-Received: by 2002:a1c:7409:: with SMTP id p9-v6mr4796522wmc.43.1530106365007; Wed, 27 Jun 2018 06:32:45 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 137-v6sm9068144wmv.28.2018.06.27.06.32.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Jun 2018 06:32:44 -0700 (PDT) Date: Wed, 27 Jun 2018 15:32:28 +0200 From: Adrien Mazarguil To: Shahaf Shuler Cc: "Xueming(Steven) Li" , "dev@dpdk.org" Message-ID: <20180627133228.GV4025@6wind.com> References: <20180525161814.13873-1-adrien.mazarguil@6wind.com> <20180614083047.10812-1-adrien.mazarguil@6wind.com> <20180614083047.10812-7-adrien.mazarguil@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v2 6/7] net/mlx5: probe all port representors X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Jun 2018 13:32:45 -0000 On Sun, Jun 17, 2018 at 10:15:07AM +0000, Shahaf Shuler wrote: > Hi Adrien, > > Saturday, June 16, 2018 11:58 AM, Xueming(Steven) Li: > > Subject: RE: [dpdk-dev] [PATCH v2 6/7] net/mlx5: probe all port representors > > > > > -----Original Message----- > > > From: dev On Behalf Of Adrien Mazarguil > > > Sent: Thursday, June 14, 2018 4:35 PM > > > To: Shahaf Shuler > > > Cc: dev@dpdk.org > > > Subject: [dpdk-dev] [PATCH v2 6/7] net/mlx5: probe all port > > > representors > > > > > > Probe existing port representors in addition to their master device and > > associate them automatically. > > > > > > To avoid name collision between Ethernet devices, their names use the > > > same convention as ixgbe and i40e PMDs, that is, instead of only a PCI > > address in DBDF notation: > > > > > > - "net_{DBDF}_0" for master/switch devices. > > This is breaking compatibility for application using the device names in order to attach them to the application (e.g. OVS-DPDK). > Before this patch the naming scheme for non-representor port is "{DBDF}". > > Can we preserve the compatibility and add appropriate suffix for the representor case? There's one issue if representors are hot-plugged. The name of the master device, which happens to be that of the switch domain, cannot be updated. The form "net_{DBDF}_0" seems expected for PMDs that support representors (see ixgbe and i40e). Now since representor hot-plugging is not supported yet, I guess we could postpone this problem by keeping the old format in the meantime, however ideally, these applications should not rely on it. The only safe assumption they can make is the uniqueness of any given name among ethdevs. PCI bus addresses, if needed, should be retrieved by looking at the underlying bus object. By the way, while thinking again about a past comment from Xueming [1], maybe it's finally time to remove support for multiple Verbs ports on mlx5 after all. This should drop another unnecessary loop and the need for the unused "port %u" suffix at all while naming the device. So how about the following plan for v3: - Adding a patch that drops support for multiple Verbs ports (note for Xueming, yes I changed my mind *again* :) - If you really think this will break OVS (please confirm), then when no "representor" parameter is provided (regardless of the presence of any representors), name format will use the usual "{DBDF}" notation as you suggested. - Otherwise as soon as a "representor" is found on the command line, the new format will be used, again regardless of the presence of any representors. - In both cases, representors if any, will be named according to the format specified in this patch. [1] https://mails.dpdk.org/archives/dev/2018-June/104015.html > > > for (i = 0; i < attr.orig_attr.phys_port_cnt; ++i) { > > > - eth_list[i] = mlx5_dev_spawn_one(dpdk_dev, ibv_dev, vf, > > > - &attr, i + 1); > > > - if (eth_list[i]) > > > - continue; > > > - /* Save rte_errno and roll back in case of failure. */ > > > - ret = rte_errno; > > > - while (i--) { > > > - mlx5_dev_close(eth_list[i]); > > > - if (rte_eal_process_type() == RTE_PROC_PRIMARY) > > > - rte_free(eth_list[i]->data->dev_private); > > > - claim_zero(rte_eth_dev_release_port(eth_list[i])); > > > - } > > > - free(eth_list); > > > - rte_errno = ret; > > > - return NULL; > > > + eth_list[n] = mlx5_dev_spawn_one(dpdk_dev, ibv_dev[j], > > vf, > > > + &attr, i + 1, > > > + j ? eth_list[0] : NULL, > > > + j - 1); > > The representor id is according to the sort made by qsort (based on device names). > A better way may be to set it according to the sysfs information, like you do in the mlx5_get_ifname function. > What do you think? I agree that the current approach sucks, hence the big fat warnings I left around (see discussion with Xueming [2]). Problem is that the needed information is not yet known at this stage; there is no private structure to rely on to use mlx5_get_ifname() directly. I'd also rather see these assumptions go in any case. I'll attempt to improve things for v3 in preparation of allowing representors to be probed on their own anytime, possibly even before the master device. [2] https://mails.dpdk.org/archives/dev/2018-June/104059.html > > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index > > > 997b04a33..0fe467140 100644 > > > --- a/drivers/net/mlx5/mlx5.h > > > +++ b/drivers/net/mlx5/mlx5.h > > > @@ -161,6 +161,10 @@ struct priv { > > > uint16_t mtu; /* Configured MTU. */ > > > uint8_t port; /* Physical port number. */ > > > unsigned int isolated:1; /* Whether isolated mode is enabled. */ > > > + unsigned int representor:1; /* Device is a port representor. */ > > Why we need above flag? Why can't we use RTE_ETH_DEV_REPRESENTOR from eth_dev->data->dev_flags. Problem is that this flag can only be set once the ethdev is fully instantiated and can't be relied on internally where needed (e.g. during clean up in error handling code). It's reported to applications but not used internally. As a device property, it's actually pretty similar to the VF bit or offloaded capabilities where checking exposed information would be needlessly complex. Now maybe it could be part of struct mlx5_dev_config as well. I initially assumed this object was only for user-provided parameters but looks like it's not the case. I intend to move it there for v3. -- Adrien Mazarguil 6WIND