From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 7/7] net/mlx5: add parameter for port representors Date: Thu, 14 Jun 2018 10:01:02 +0200 Message-ID: <20180614080102.GL4025@6wind.com> References: <20180525161814.13873-1-adrien.mazarguil@6wind.com> <20180525161814.13873-8-adrien.mazarguil@6wind.com> <20180612132043.GZ4025@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Shahaf Shuler , "dev@dpdk.org" To: "Xueming(Steven) Li" Return-path: Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by dpdk.org (Postfix) with ESMTP id 4A5DF1EADA for ; Thu, 14 Jun 2018 10:01:19 +0200 (CEST) Received: by mail-wr0-f196.google.com with SMTP id w10-v6so5326016wrk.9 for ; Thu, 14 Jun 2018 01:01:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jun 12, 2018 at 01:43:18PM +0000, Xueming(Steven) Li wrote: > > > > void *tmp; > > > > unsigned int i; > > > > unsigned int j = 0; > > > > unsigned int n = 0; > > > > int ret; > > > > > > > > + if (dpdk_dev->devargs) { > > > > + ret = rte_eth_devargs_parse(dpdk_dev->devargs->args, ð_da); > > > > + if (ret) > > > > + goto error; > > > > + } else { > > > > + memset(ð_da, 0, sizeof(eth_da)); > > > > + } > > > > next: > > > > + if (j) { > > > > + unsigned int k; > > > > + > > > > + for (k = 0; k < eth_da.nb_representor_ports; ++k) > > > > + if (eth_da.representor_ports[k] == j - 1) > > > > + break; > > > > + if (k == eth_da.nb_representor_ports) > > > > + goto skip; > > > > + } > > > > errno = 0; > > > > ctx = mlx5_glue->open_device(ibv_dev[j]); > > > > > > Need a range check for j here. > > > > I think it's properly checked. j == 0 stands for "master device", always found at index 0 and probed. > > Representors devices, if any, start at index 1 which triggers the previous block. This block makes > > sure that a given representor is indeed enabled before either spawning the related device (pass > > through with a valid "j") or skipping it altogether (goto skip). > > Yes, this code looks good. What I wanted to ask what if dev args specify an invalid rep id, e.g. 33. > This code walk through silently w/o warning, it works, but it better to have a warning if input id out of range. You're right. On the other hand this provides a means to spawn all representors without necessarily knowing how many can be instantiated first, e.g. by always providing a "representor=[0-31]" argument, since no special keyword is defined to request them all. Not saying it's a good or bad thing, but somewhat harmless. Just like specifying "-w {DBDF}" arguments with invalid addresses, nonexistent representors are silently ignored. In any case, this can be improved later. We're already seeing a couple of limitations with the representor argument, namely the lack of hot-plug support, which will need to be addressed as well. -- Adrien Mazarguil 6WIND