From mboxrd@z Thu Jan 1 00:00:00 1970 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 08457271 for ; Mon, 18 Dec 2017 18:59:13 +0100 (CET) Received: by mail-wr0-f196.google.com with SMTP id l41so2892665wre.11 for ; Mon, 18 Dec 2017 09:59:13 -0800 (PST) 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:content-transfer-encoding:in-reply-to; bh=hw5jcOx34/UrAcy+8ACzIQa5V0jOYrhywSga3u9zUWg=; b=ZMjkR4pEe/gfdGk3ReMdA0y375LhIGfKdCD5jIXrz17HwCmr3yD8bjFCOi7DrUYk/R xeJznsVUnipMRkAH/o5Nb+kW7Ca7oKNF7k9tkU3yqglMy+3l8wQQ8LpEyDPioWnnvCeZ NKjMKtLsY9dhlYrdLphwV4dw80kLoBpu7yfD8Yy4RExCIkY9EpBlQCyZvHCeAoxlBALs Lnf6iPGk7QttTPoov0hH6lsaKnVbIMvyyo1I+QQ/+lJPQl7GeLcaOd9a8DvSomfSTOvi kQ6wEBcDKnl01T0lykaZLhQKTB9qfK/uo0p7RFh6/7EbhtxLwBTv9Y8YLdZRkykwDJd3 9RhA== 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:content-transfer-encoding :in-reply-to; bh=hw5jcOx34/UrAcy+8ACzIQa5V0jOYrhywSga3u9zUWg=; b=W1Mlh4sek2BSxFnIolm+V2zHvnrAG6i+awTFzNb2OuIm8i84gMoc4zYakCU3aBkivx v3MM4uPHpiAzTaVvwfvr8MvNRdgjF14Zk3Lj18gbeaLUN8J18rZOwSk+WGKCL2nw9bf/ W/0lFJcojvjpU7FJ5ZZGyBtpcAbnR6yQoDbUKV2GeHgZylnSvZ7dex44lNS2eNGPgt9a trydoNlK+w0CRRq61/FET7eFW1hfvLqydFukoYCrvI4S1osOzFX2lJXEd45zceGMItTb YyMRb+3gy2eURrN3ea9SBn6NRuRAStCOq0HMoWtJl+84InXPa5T4LGK9v8s+QLxdVEJe 9Vqg== X-Gm-Message-State: AKGB3mKoEMc7FJa571bhBEjYlUhqE/2GnOAeZEfv2UsjJQiiSlIJmyxG fteflxnZTPyuWT1LaueCDgnbAw== X-Google-Smtp-Source: ACJfBosfNpm6ZEF5K/Jl9jx8xUJjNwnWGX5LpbHSs67G231YmczGVDtJxYJwXYF26GTZ4uXzJgI/Iw== X-Received: by 10.223.136.243 with SMTP id g48mr778392wrg.247.1513619952666; Mon, 18 Dec 2017 09:59:12 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id p13sm15074471wrc.61.2017.12.18.09.59.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 18 Dec 2017 09:59:11 -0800 (PST) Date: Mon, 18 Dec 2017 18:59:00 +0100 From: Adrien Mazarguil To: "Wiles, Keith" Cc: "Yigit, Ferruh" , "dev@dpdk.org" , Stephen Hemminger Message-ID: <20171218175900.GC4062@6wind.com> References: <20171124172132.GW4062@6wind.com> <20171218162443.12971-1-adrien.mazarguil@6wind.com> <20171218162443.12971-3-adrien.mazarguil@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v1 2/3] net/hyperv: implement core functionality 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: Mon, 18 Dec 2017 17:59:13 -0000 On Mon, Dec 18, 2017 at 05:04:23PM +0000, Wiles, Keith wrote: > > On Dec 18, 2017, at 10:46 AM, Adrien Mazarguil wrote: > > > > As described in more details in the attached documentation (see patch > > contents), this virtual device driver manages NetVSC interfaces in virtual > > machines hosted by Hyper-V/Azure platforms. > > > > This driver does not manage traffic nor Ethernet devices directly; it acts > > as a thin configuration layer that automatically instantiates and controls > > fail-safe PMD instances combining tap and PCI sub-devices, so that each > > NetVSC interface is exposed as a single consolidated port to DPDK > > applications. > > > > PCI sub-devices being hot-pluggable (e.g. during VM migration), > > applications automatically benefit from increased throughput when present > > and automatic fallback on NetVSC otherwise without interruption thanks to > > fail-safe's hot-plug handling. > > > > Once initialized, the sole job of the hyperv driver is to regularly scan > > for PCI devices to associate with NetVSC interfaces and feed their > > addresses to corresponding fail-safe instances. > > > > Signed-off-by: Adrien Mazarguil > > --- > > doc/guides/nics/hyperv.rst | 65 ++++ > > drivers/net/hyperv/Makefile | 4 + > > drivers/net/hyperv/hyperv.c | 654 ++++++++++++++++++++++++++++++++++++++- > > 3 files changed, 722 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/hyperv/hyperv.c b/drivers/net/hyperv/hyperv.c > > index 2f940c76f..bad224be9 100644 > > --- a/drivers/net/hyperv/hyperv.c > > +++ b/drivers/net/hyperv/hyperv.c > > +/** > > + * Convert a MAC address string to binary form. > > + * > > + * Note: this function should be exposed by rte_ether.h as the reverse of > > + * ether_format_addr(). > > + * > > + * Several MAC string formats are supported on input for convenience: > > + * > > + * 1. "12:34:56:78:9a:bc" > > + * 2. "12-34-56-78-9a-bc" > > + * 3. "123456789abc" > > + * 4. Upper/lowercase hexadecimal. > > + * 5. Any combination of the above, e.g. "12:34-5678-9aBC". > > + * 6. Partial addresses are allowed, with low-order bytes filled first: > > + * - "5:6:78c" translates to "00:00:05:06:07:8c", > > + * - "5678c" translates to "00:00:00:05:67:8c". > > + * > > + * Non-hexadecimal characters, unknown separators and strings specifying > > + * more than 6 bytes are not allowed. > > + * > > + * @param[out] eth_addr > > + * Pointer to conversion result buffer. > > + * @param[in] str > > + * MAC address string to convert. > > + * > > + * @return > > + * 0 on success, -EINVAL in case of unsupported format. > > + */ > > +static int > > +ether_addr_from_str(struct ether_addr *eth_addr, const char *str) > > +{ > > + static const uint8_t conv[0x100] = { > > + ['0'] = 0x80, ['1'] = 0x81, ['2'] = 0x82, ['3'] = 0x83, > > + ['4'] = 0x84, ['5'] = 0x85, ['6'] = 0x86, ['7'] = 0x87, > > + ['8'] = 0x88, ['9'] = 0x89, ['a'] = 0x8a, ['b'] = 0x8b, > > + ['c'] = 0x8c, ['d'] = 0x8d, ['e'] = 0x8e, ['f'] = 0x8f, > > + ['A'] = 0x8a, ['B'] = 0x8b, ['C'] = 0x8c, ['D'] = 0x8d, > > + ['E'] = 0x8e, ['F'] = 0x8f, [':'] = 0x40, ['-'] = 0x40, > > + ['\0'] = 0x60, > > + }; > > + uint64_t addr = 0; > > + uint64_t buf = 0; > > + unsigned int i = 0; > > + unsigned int n = 0; > > + uint8_t tmp; > > + > > + do { > > + tmp = conv[(int)*(str++)]; > > + if (!tmp) > > + return -EINVAL; > > + if (tmp & 0x40) { > > + i += (i & 1) + (!i << 1); > > + addr = (addr << (i << 2)) | buf; > > + n += i; > > + buf = 0; > > + i = 0; > > + } else { > > + buf = (buf << 4) | (tmp & 0xf); > > + ++i; > > + } > > + } while (!(tmp & 0x20)); > > + if (n > 12) > > + return -EINVAL; > > + i = RTE_DIM(eth_addr->addr_bytes); > > + while (i) { > > + eth_addr->addr_bytes[--i] = addr & 0xff; > > + addr >>= 8; > > + } > > + return 0; > > +} > > You already called this out above, why not just push this into rte_ether.h file. I know I could use it if it were public. Hehe, that was to highlight how this driver didn't require any modifications in public APIs. I planned to do just that in v2 or in a subsequent patch. > > +/** > > + * Retrieve the last component of a path. > > + * > > + * This is a simplified basename() that does not modify its input buffer to > > + * handle trailing backslashes. > > + * > > + * @param[in] path > > + * Path to retrieve the last component from. > > + * > > + * @return > > + * Pointer to the last component. > > + */ > > +static const char * > > +hyperv_basename(const char *path) > > +{ > > + const char *tmp = path; > > + > > + while (*tmp) > > + if (*(tmp++) == '/') > > + path = tmp; > > + return path; > > +} > > Why not just user rindex() to find the last ‘/‘ instead of this routine? I know it is not performance critical. Right, however both rindex() and strrchr() return NULL when no '/' is present. strchrnul() works but is GNU-specific (i.e. probably not found on BSD), I didn't want to perform an additional check for that, so actually given the size of that function I didn't give it a second thought. I can modify that if needed. > > +/** > > + * Probe a network interface to associate with hyperv context. > > + * > > + * This function determines if the network device matches the properties of > > + * the NetVSC interface associated with the hyperv context and communicates > > + * its bus address to the fail-safe PMD instance if so. > > + * > > + * It is normally used with hyperv_foreach_iface(). > > + * > > + * @param[in] iface > > + * Pointer to netdevice description structure (name and index). > > + * @param[in] eth_addr > > + * MAC address associated with @p iface. > > + * @param ap > > + * Variable arguments list comprising: > > + * > > + * - struct hyperv_ctx *ctx: > > + * Context to associate network interface with. > > + * > > + * @return > > + * A nonzero value when interface matches, 0 otherwise or in case of > > + * error. > > + */ > > +static int > > +hyperv_device_probe(const struct if_nameindex *iface, > > + const struct ether_addr *eth_addr, > > + va_list ap) > > +{ > > + struct hyperv_ctx *ctx = va_arg(ap, struct hyperv_ctx *); > > + char buf[RTE_MAX(sizeof(ctx->yield), 256u)]; > > + const char *addr; > > + size_t len; > > + int ret; > > + > > + /* Skip non-matching or unwanted NetVSC interfaces. */ > > + if (ctx->if_index == iface->if_index) { > > + if (!strcmp(ctx->if_name, iface->if_name)) > > + return 0; > > + DEBUG("NetVSC interface \"%s\" (index %u) renamed \"%s\"", > > + ctx->if_name, ctx->if_index, iface->if_name); > > + strncpy(ctx->if_name, iface->if_name, sizeof(ctx->if_name)); > > + return 0; > > + } > > + if (hyperv_iface_is_netvsc(iface)) > > + return 0; > > + if (!is_same_ether_addr(eth_addr, &ctx->if_addr)) > > + return 0; > > + /* Look for associated PCI device. */ > > + ret = hyperv_sysfs_readlink(buf, sizeof(buf), iface->if_name, > > + "device/subsystem"); > > + if (ret) > > + return 0; > > + if (strcmp(hyperv_basename(buf), "pci")) > > + return 0; > > + ret = hyperv_sysfs_readlink(buf, sizeof(buf), iface->if_name, > > + "device"); > > + if (ret) > > + return 0; > > + addr = hyperv_basename(buf); > > + len = strlen(addr); > > + if (!len) > > + return 0; > > + /* Send PCI device argument to fail-safe PMD instance if updated. */ > > + if (!strcmp(addr, ctx->yield)) > > + return 1; > > + DEBUG("associating PCI device \"%s\" with NetVSC interface \"%s\"" > > + " (index %u)", > > + addr, ctx->if_name, ctx->if_index); > > + memmove(buf, addr, len + 1); > > + addr = buf; > > + buf[len] = '\n'; > > + ret = write(ctx->pipe[1], addr, len + 1); > > + buf[len] = '\0'; > > + if (ret == -1) { > > + if (errno == EINTR || errno == EAGAIN) > > + return 1; > > + WARN("cannot associate PCI device name \"%s\" with interface" > > + " \"%s\": %s", > > + addr, ctx->if_name, rte_strerror(errno)); > > + return 1; > > + } > > + if ((size_t)ret != len + 1) { > > + /* > > + * Attempt to override previous partial write, no need to > > + * recover if that fails. > > + */ > > + ret = write(ctx->pipe[1], "\n", 1); > > + (void)ret; > > + return 1; > > + } > > + fsync(ctx->pipe[1]); > > + memcpy(ctx->yield, addr, len + 1); > > + return 1; > > +} > > Not to criticize style, but a few blank lines could help in readability for these files IMHO. Unless blank lines are illegal :-) It's a matter of taste, I think people tend to add random blank lines where they think doing so clarifies things for themselves, resulting in inconsistent coding style not much clearer for everyone after several iterations. As a maintainer I've grown tired of discussions related to blank lines while reviewing patches. That's why except for a few special cases, I now enforce exactly the bare minimum of one blank line between variable declarations and the rest of the code inside each block. If doing so makes a function unreadable then perhaps it needs to be split :) I'm sure you'll understand! Regards, -- Adrien Mazarguil 6WIND