From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id 64BBA3253 for ; Mon, 17 Jul 2017 19:11:30 +0200 (CEST) Received: by mail-wm0-f50.google.com with SMTP id w126so82396423wme.0 for ; Mon, 17 Jul 2017 10:11:30 -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:content-transfer-encoding:in-reply-to :user-agent; bh=oVHbT/AlpS8Pq9Lb0J4MfTxyNbJFG51gGCWMn4sz4oo=; b=UoEFFqcQo/T4scubUBGXrDQlC/W5SVf/HxsOQr9tuRaJm7SRfGixwCqKdmktth3p/P 6G5G0QRi3A7XNoyvWLgUh3QO2B+mgf7WQY6x6Y6EXY6Fai/JHix1q41RNPolZjQkSSvM B0CiOjDYRi99X79rI5F9WsNy7lwY73LtIpq2zdc6DZeXq5KelaY8ekKVzGbE/bWLtER0 J7I0NZR1Iqa4i5V0DeY9YEx+lsKD73dyy4DtHRTRf3z6zZr/ulWKFcYq7nXkKzn8Fbx5 +SaBYoXamCTgS1nC5+Rh4R8Sq8JU1cSZLtpQomPDc0UnBRB2OOgeUUd52SGLvH6bvtRX V1wg== 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:user-agent; bh=oVHbT/AlpS8Pq9Lb0J4MfTxyNbJFG51gGCWMn4sz4oo=; b=EPVRPRGIvXGeguJ5r+b1xMJ05vdcslh9jSNJdCl27Uydoc5E2gfJGPKSR83sEgkLw6 EloHnc7SCRzMehDqb2takHY1yMXru64VccR0IJE5z0cSLn0Qs/1jHtJeI6MaOD9rSVMJ 67WAxj7/4k3yneadTpXBfOwywOdwz9rW/+vxK1dvTRVolU3j+H+wsg0eYaKGENvSsNk2 pt7DBUxYqwJdzYAyOzbJUhaGnDm0NIz6cQoWyPH1kf1HG2aVcKQgrZ86B6mw1xDYJPli ddj3pNQBQl4P32A3+95eDyXOCK0L7EVzkpa4SkO2FINWQRzZ6QkkQkrlP/x875iJi7w+ PMjQ== X-Gm-Message-State: AIVw11129mPWOH+SVPEIo03XitmO+/Y/rxdMwR6JpULmHQO2zwyTenX2 hZGdPYw6zfK0v0/z X-Received: by 10.80.140.204 with SMTP id r12mr18231108edr.123.1500311489904; Mon, 17 Jul 2017 10:11:29 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id g55sm93887edg.66.2017.07.17.10.11.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 17 Jul 2017 10:11:29 -0700 (PDT) Date: Mon, 17 Jul 2017 19:11:20 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Ferruh Yigit Cc: dev@dpdk.org Message-ID: <20170717171120.GS11154@bidouze.vm.6wind.com> References: <7390b7f14a1925ece0c55c6b1df8da358c725017.1500130634.git.gaetan.rivet@6wind.com> <784ab02a-2454-f209-ac52-ff1e53ec1a63@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <784ab02a-2454-f209-ac52-ff1e53ec1a63@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v10 03/11] net/failsafe: add fail-safe PMD 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, 17 Jul 2017 17:11:30 -0000 On Mon, Jul 17, 2017 at 02:56:54PM +0100, Ferruh Yigit wrote: > On 7/15/2017 6:57 PM, Gaetan Rivet wrote: > > Introduce the fail-safe poll mode driver initialization and enable its > > build infrastructure. > > > > This PMD allows for applications to benefit from true hot-plugging > > support without having to implement it. > > > > It intercepts and manages Ethernet device removal events issued by > > slave PMDs and re-initializes them transparently when brought back. > > It also allows defining a contingency to the removal of a device, by > > designating a fail-over device that will take on transmitting operations > > if the preferred device is removed. > > > > Applications only see a fail-safe instance, without caring for > > underlying activity ensuring their continued operations. > > All PMD in a single patch is hard to review, I am sure some details > missed during the review, but taking account the histroy of the PMD I > accept this as it is, but I will rely on your support to fix issues in > the future. > Sure, sorry for having this one first big patch. I thought about having a skeleton patch first, but found it made little sense. I tried to restrict this version to the bare functionalities, adding the others afterward. I will fix any issues. From what I've seen I agree with almost all of your remarks and will send a new version shortly. In the meantime, I will answer in this email a few clarifying questions. <...> > > +VLAN filter = Y > > +Packet type parsing = Y > > I am not sure how to document some of these features, because they > depends on sub-device capability. I guess if sub-device doesn't support > packet type parsing, this feature won't be supported? > Yes, supporting a feature for the fail-safe means that there is some verification and synchronization code related to this feature. All sub_device should have feature parity, and the features of the fail-safe are limited to those of the sub_devices. I thought advertizing the support made sense as there was some code related to it in the fail-safe. > > +int > > +failsafe_args_parse(struct rte_eth_dev *dev, const char *params) > > +{ > > + struct fs_priv *priv; > > + char mut_params[DEVARGS_MAXLEN] = ""; > > Out of curiosity, what does "mut" stands for? > This is the mutable version of params. <...> > > + n = snprintf(mut_params, sizeof(mut_params), "%s", params); > > + if (n >= sizeof(mut_params)) { > > + ERROR("Parameter string too long (>=%zu)", > > + sizeof(mut_params)); > > + return -ENOMEM; > > + } > > + ret = fs_parse_sub_devices(fs_parse_device_param, > > + dev, params); > > Why the device argument is not defined as dev=xxx, instead of current > dev(xxx). > > "dev=xxx" will be compatible with rest of the argument usage, and it > will be possible to use kvargs to parse it, which will make this code > simpler I believe. > > What is the reason of using different syntax? > Using the dev() syntax allows the user to explicitly set the limits of the sub_device declaration, clarifying for which device each kvargs is. The issue is that the kvargs library does not allow to set state informations in the parser depending on the position in the kvlist. An alternative would have been for example to restrict the kvargs to that of the last declared dev=, however, this means multi-stage kvargs parsing, which mean pre-processing of the parameter list, etc... An example: net_failsafe0,dev=net_tap0,iface=tap0,mac=00:01:02:03:04:05 net_failsafe0,dev(net_tap0,iface=tap0),mac=00:01:02:03:04:05 This is much simpler to parse this way, and much clearer I think for users. The kvargs library was not designed with recursive PMDs in mind. <...> > > +static int > > +fs_bus_init(struct rte_eth_dev *dev) > > +{ > > + struct sub_device *sdev; > > + struct rte_devargs *da; > > + uint8_t i; > > + int ret; > > + > > + FOREACH_SUBDEV(sdev, i, dev) { > > Can FOREACH_SUBDEV_ST(..., DEV_PARSED) be used here? > I could use it, this would restrict the iteration only to sub_devices being at least of the state DEV_PARSED. However, in the check just below: + if (sdev->state != DEV_PARSED) + continue; I would have to pass on any device being in a state higher than DEV_PARSED. Thus, using FOREACH_SUBDEV_ST would not simplify the code flow. By using FOREACH_SUBDEV() directly, the reader at least has a simpler parsing to do of my intent: foreach subdev not "parsed". > And what do you think renaming "FOREACH_SUBDEV_ST" to > "FOREACH_SUBDEV_STATE"? > Sure, I pushed for brievity but it might be easier to read. <...> > > + FOREACH_SUBDEV_ST(sdev, i, dev, DEV_ACTIVE) { > > + DEBUG("Closing sub_device %d", i); > > + rte_eth_dev_close(PORT_ID(sdev)); > > + sdev->state = DEV_ACTIVE - 1; > > Should it be better to set state to DEV_PROBED? Instead of calculation. > I wanted to be able to add / remove device states without having to rewrite each of those state changes (there are a few in several places). If I insert a new device state between ACTIVE and PROBED, setting to DEV_PROBED would still be valid (no compile error), but it would be a bug. It would be very easy to miss a reference to this specific state. Those states bugs are a little hard to find at runtime, they usually have subtle side-effects. I can change it if you prefer, but I would probably introduce a helper in the form of fs_dev_state_prev/next, thus having a single place to check for any changes. -- Gaëtan Rivet 6WIND