From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id CD7AC7EC7 for ; Tue, 10 Jul 2018 11:37:25 +0200 (CEST) Received: by mail-wm0-f67.google.com with SMTP id s14-v6so23604254wmc.1 for ; Tue, 10 Jul 2018 02:37:25 -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=CNb8Ze+qHuoRx8ana1CHG67yc86kDeJ5mfyFNmRUN4k=; b=Am3uCtgYi6n10kuGLRiNZnQcrnRTL/+TzFeUB1Bg05HPD2NKgwV4DJ1xwziqsrRTkV Ij+UNo7PGgEUKqQpwB7ThSoyOZIXrSOuOUQRF6Q/jS76PSLmz0x3DpiPL5u4w/lZ7NQg pqojxbGr9zWUznrIzRDdsPryqXMW8YIzHsveXMhYTg115OVBJ3JB4DmSIYgKsUC9ift7 1M9E1eTWeXBhiA/NlbvdOWXW0UTIi83mVB/lbUUq9BblVzBI3jxEkVOhJgpElmARSNPK 2t5eD+pTQFzXvCH+vVylXqRkjw0TF5s+vCXwDDz8hdhN3sjyUJxZuE3QjjKNXM0+QSFw uF9Q== 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=CNb8Ze+qHuoRx8ana1CHG67yc86kDeJ5mfyFNmRUN4k=; b=Z6xqldzD0zC30kfN0hCkmmlpQux2rmdoaaoQxD9XLmUJR1lbYONqdBrNVrMJZMQNBG 7N7SIux5fJX7UmYamwXQg3lQtyPx1qq+Q6jawnmGL0efVBqGBg4LBPTdGU/VPGNSAC70 rkrQ2APGYGohNi7+Xu0WDH6aSI2Qvs7C++ps7ZXj2uJlinPzqMd8W651kfPpu63y1CKU nPsziSIdtYrWjtCzCx/N7HfR8CyYA5z/1fWZwFvLZVPmrItnk898n+UOXhrKvL0izWXo F8WfZv5foyF67+cQ6GfhvEhDcGrOrQq+T3K/uKbLIWlEDhqKTywGmUhL6YVzpsds1WdU x7ZQ== X-Gm-Message-State: APt69E3o6WZBASgkKPQgcj4fw4DJdGuE5XvKV+9m73mqKAWPlCHkN6wI CaehKVdeDF4EF8ld8wvoK0ZEEA== X-Google-Smtp-Source: AAOMgpcxAnTrTFOC/yIJho4mcfeyNlUrTORl0DlmtB2mbfNYqwBT1TzD8EPLSeN0gCKSE/nNOJob2g== X-Received: by 2002:a1c:f009:: with SMTP id a9-v6mr14997608wmb.104.1531215445588; Tue, 10 Jul 2018 02:37:25 -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 g11-v6sm11470619wrm.31.2018.07.10.02.37.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 10 Jul 2018 02:37:24 -0700 (PDT) Date: Tue, 10 Jul 2018 11:37:08 +0200 From: Adrien Mazarguil To: Shahaf Shuler Cc: "dev@dpdk.org" , =?utf-8?B?TsOpbGlv?= Laranjeiro , "Xueming(Steven) Li" Message-ID: <20180710093708.GH5211@6wind.com> References: <20180704172322.22571-1-adrien.mazarguil@6wind.com> <20180705083934.5535-1-adrien.mazarguil@6wind.com> <20180705083934.5535-8-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 v4 07/10] 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: Tue, 10 Jul 2018 09:37:25 -0000 On Mon, Jul 09, 2018 at 11:57:29AM +0000, Shahaf Shuler wrote: > Hi Adrien, > > > Thursday, July 5, 2018 11:46 AM, Adrien Mazarguil: > > Subject: [PATCH v4 07/10] net/mlx5: probe all port representors > > > > Probe existing port representors in addition to their master device and > > associate them automatically. > > > > To avoid collision between Ethernet devices, they are named as follows: > > > > - "{DBDF}" for master/switch devices. > > - "{DBDF}_representor_{rep}" with "rep" starting from 0 for port > > representors. > > > > (Patch based on prior work from Yuanhan Liu) > > > > Signed-off-by: Adrien Mazarguil > > Signed-off-by: Nelio Laranjeiro > > Reviewed-by: Xueming Li > > Cc: Xueming Li > > Cc: Shahaf Shuler > > -- > > v4 changes: > > > > - Fixed domain ID release once the last port using it is closed. Closed > > devices are not necessarily detached, their presence is not a good > > indicator. Code was modified to check if they still use their domain IDs > > before deciding to release it. > > @@ -883,6 +915,41 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev, > > priv->nl_socket_rdma = mlx5_nl_init(0, NETLINK_RDMA); > > priv->nl_socket_route = mlx5_nl_init(RTMGRP_LINK, > > NETLINK_ROUTE); > > priv->nl_sn = 0; > > + priv->representor = !!switch_info->representor; > > + priv->domain_id = RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID; > > + priv->representor_id = > > + switch_info->representor ? switch_info->port_name : -1; > > + /* > > + * Look for sibling devices in order to reuse their switch domain > > + * if any, otherwise allocate one. > > + */ > > + i = mlx5_dev_to_port_id(dpdk_dev, NULL, 0); > > + if (i > 0) { > > + uint16_t port_id[i]; > > + > > + i = RTE_MIN(mlx5_dev_to_port_id(dpdk_dev, port_id, i), i); > > + while (i--) { > > + const struct priv *opriv = > > + rte_eth_devices[port_id[i]].data- > > >dev_private; > > + > > + if (!opriv || > > + opriv->domain_id == > > + RTE_ETH_DEV_SWITCH_DOMAIN_ID_INVALID) > > + continue; > > + priv->domain_id = opriv->domain_id; > > It looks like for the second port it will use the domain_id of the first port. Is that what you intent? Yes, it's on purpose. Master and representors of a given device must share the same domain ID to let applications know they can create flow rules to forward traffic between them all. > Note - I couldn't test it due to compilation errors: > > /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5_nl.c: In function 'mlx5_nl_switch_info_cb': > /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5_nl.c:843:8: error: 'IFLA_PHYS_PORT_NAME' undecl > ared (first use in this function) > case IFLA_PHYS_PORT_NAME: > ^ > /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5_nl.c:843:8: note: each undeclared identifier is > reported only once for each function it appears in > /.autodirect/swgwork/shahafs/workspace/dpdk.org/drivers/net/mlx5/mlx5_nl.c:851:8: error: 'IFLA_PHYS_SWITCH_ID' undecl > ared (first use in this function) > case IFLA_PHYS_SWITCH_ID: > ^ > > My system info: > NAME="Red Hat Enterprise Linux Server" > VERSION="7.3 (Maipo)" > ID="rhel" > ID_LIKE="fedora" > VERSION_ID="7.3" > PRETTY_NAME="Red Hat Enterprise Linux Server 7.3 (Maipo)" > ANSI_COLOR="0;31" > CPE_NAME="cpe:/o:redhat:enterprise_linux:7.3:GA:server" > HOME_URL="https://www.redhat.com/" > BUG_REPORT_URL="https://bugzilla.redhat.com/" > > REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 7" > REDHAT_BUGZILLA_PRODUCT_VERSION=7.3 > REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux" > REDHAT_SUPPORT_PRODUCT_VERSION="7.3" > Red Hat Enterprise Linux Server release 7.3 (Maipo) > Red Hat Enterprise Linux Server release 7.3 (Maipo) OK, I'll redefine in v5 in case they are missing on the host system. > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index > > 704046270..cc01310e0 100644 > > --- a/drivers/net/mlx5/mlx5.h > > +++ b/drivers/net/mlx5/mlx5.h > > @@ -159,6 +159,7 @@ struct priv { > > struct ibv_context *ctx; /* Verbs context. */ > > struct ibv_device_attr_ex device_attr; /* Device properties. */ > > struct ibv_pd *pd; /* Protection Domain. */ > > + char ibdev_name[IBV_SYSFS_NAME_MAX]; /* IB device name. */ > > > Why we need a dedicated entry for the ibdev_name? it is already part of priv->ctx->device->name. Heh, same reason as the next line below, don't forget those damn secondaries which can't dereference local pointers from the primary process :) > > char ibdev_path[IBV_SYSFS_PATH_MAX]; /* IB device path for > > secondary */ > > struct rte_eth_dev_info *info) > > info->speed_capa = priv->link_speed_capa; > > info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK; > > mlx5_set_default_params(dev, info); > > + info->switch_info.name = dev->data->name; > > + info->switch_info.domain_id = priv->domain_id; > > + info->switch_info.port_id = priv->representor_id; > > + if (priv->representor) { > > + unsigned int i = mlx5_dev_to_port_id(dev->device, NULL, 0); > > + uint16_t port_id[i]; > > + > > + i = RTE_MIN(mlx5_dev_to_port_id(dev->device, port_id, i), > > i); > > + while (i--) { > > + struct priv *opriv = > > + rte_eth_devices[port_id[i]].data- > > >dev_private; > > + > > + if (!opriv || > > + opriv->representor || > > + opriv->domain_id != priv->domain_id) > > + continue; > > + /* > > + * Override switch name with that of the master > > + * device. > > + */ > > + info->switch_info.name = opriv->dev_data->name; > > + break; > > According to this logic it means once the master device is closed, all the representors are no longer belong to the same switch (switch name of each is different) which is not correct. They still share the same domain ID, which is what actually matters. The switch name is only provided to let applications identify the master (control) device in case it's needed. > According to your notes it is possible to close master w/o closing the representor. This allows devices to be probed in any order on a needed basis, not all at once. It's done on purpose to pave the way for hotplug support. > Why not just storing the master switch name when probing the representor and to use it as is on the dev_info? The switch name *must* be that of the master device. If the master is not probed, there can't be a switch name. However there's no real provision for this in the API, so I chose the most acceptable unique name, which is the name of the local device. Would you prefer an empty name instead? Thing is, on mlx5 flow rules can be created directly between representors without involving the master device. An empty switch name may be misleading in this respect. What do you suggest? -- Adrien Mazarguil 6WIND