From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 62AF6A00E6 for ; Thu, 18 Apr 2019 18:46:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3A28B1BC0D; Thu, 18 Apr 2019 18:46:27 +0200 (CEST) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id 726821BC08 for ; Thu, 18 Apr 2019 18:46:26 +0200 (CEST) Received: by mail-wr1-f68.google.com with SMTP id k17so3734924wrx.10 for ; Thu, 18 Apr 2019 09:46:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=/Ze56Mc2xvl84T9sJ4qMxhHT/tYKFIy74paDhasWYag=; b=iiheV2kdTfyt97qz2d401nYRUilfJyvSegOUXKD6LONmFDWsAzbEFN0VlPHVJL78bc Cf0fFfyPXf7ZdspqhQA4houkHpzzX15VJddgoPfiFyrlvlW4Qqg6n+/ucDlhRnRXJpVC gB18NGPkxntD9OkjLdWfHhy/Kde6VcqYjrpcUpvjzltCmzrCqWrCR7GpAj4ERz7Jchan Yow04ZsmOyzEpIt2Y48u00foDCrWHM34pDuqKf8tzAk13N28F0pLl/TBaxzclQ5OG7/4 3RZNwHNwqrexqLdUvkqwf4sZfCkCFR1JLHZM6ZO+/HG+VP/on2eoNy+XY+gUIyiiWNKv EH1A== 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=/Ze56Mc2xvl84T9sJ4qMxhHT/tYKFIy74paDhasWYag=; b=F1XUjAs7FwzxuDs1RQnZCbr1ua8V+PGpSEIfvmE9HuBKEsm8onAW3/E/yo+sZ6M1DO Bd/urvyxkITqL9CbSKQcAuHiEztlTRVVUGbRgxJ9U7Fi6LuRWSPDwTWeoXvxTibOW4mj iQTwzSdOlsYlC7jx7sNJkhKX6HcGFGWwzlxtFrXN8zj6XlwBJ0Q6XCHLBpUNlo4J578X dOcL2UKpY+SSrF/47y0tNohsQHnyhw49lc/kndXv+Qts8fbAieNm8hvO3yBPWS8m8Qvg wa2eUuj2WJWm4xG2/U64sXDKw78PAuEuSsZ/LRs4/voMrLjiCJ3Em0ItMENs21vYP8Zy GdJw== X-Gm-Message-State: APjAAAUbrdv+xQvfx9Qman0vQIeUrMjzcYpZ3aqFb8ajOCfAKzsVA920 D0rCug6TlCeIGQK3olrNZjooqQ== X-Google-Smtp-Source: APXvYqyXvUm8nJwiLBlwbFeQmoAntnEo6vuefaBMaAVgAppuMILbopmOa4O2g71KGSCvT4kPj2PODQ== X-Received: by 2002:adf:97c5:: with SMTP id t5mr1424385wrb.252.1555605986228; Thu, 18 Apr 2019 09:46:26 -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 d16sm3205870wrs.68.2019.04.18.09.46.25 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 18 Apr 2019 09:46:25 -0700 (PDT) Date: Thu, 18 Apr 2019 18:46:24 +0200 From: Adrien Mazarguil To: Thomas Monjalon Cc: dev@dpdk.org, Gaetan Rivet , Ferruh Yigit , David Marchand , stable@dpdk.org Message-ID: <20190418164624.GG4889@6wind.com> References: <20190418130419.25675-1-adrien.mazarguil@6wind.com> <20190418152229.13554-1-adrien.mazarguil@6wind.com> <8815526.EAVJVpUmGC@xps> <2732210.eFvFzSlaYO@xps> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <2732210.eFvFzSlaYO@xps> Subject: Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190418164624.6qVL_4f-Ha29yrYG_zdKQ6hvdVIBsjo96Hye2dmPNzE@z> On Thu, Apr 18, 2019 at 05:51:18PM +0200, Thomas Monjalon wrote: > 18/04/2019 17:39, Thomas Monjalon: > > 18/04/2019 17:32, Adrien Mazarguil: > > > When passed to the application, Rx packets retain the port ID value > > > originally set by slave devices. Unfortunately these IDs have no meaning to > > > applications, which are typically unaware of their existence. > > > > > > This confuses those caring about the source port field in mbufs (m->port) > > > which experience issues ranging from traffic drop to crashes. > [...] > > > +/* > > > + * Override source port in Rx packets. > > > + * > > > + * Make Rx packets originate from this PMD instance instead of one of its > > > + * slaves. This is mandatory to avoid breaking applications. > > > + */ > > "slave" is a wording from bonding. > > In failsafe, it is sub-device, isn't it? I don't mind, although grep shows a couple of comments talking about slaves already. Either way I think it fits as those are failsafe's pets, as in failsafe does whatever it wants to them and they don't have a say :) Does it warrant a v3? > > > +static void > > > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port) > > > +{ > > > + unsigned int i; > > > + > > > + for (i = 0; i != nb_pkts; ++i) > > > + rx_pkts[i]->port = port; > > > +} > > > + > > > uint16_t > > > failsafe_rx_burst(void *queue, > > > struct rte_mbuf **rx_pkts, > > > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue, > > > sdev = sdev->next; > > > } while (nb_rx == 0 && sdev != rxq->sdev); > > > rxq->sdev = sdev; > > > + if (nb_rx) > > > + failsafe_rx_set_port(rx_pkts, nb_rx, > > > + rxq->priv->data->port_id); > > > return nb_rx; > > > } > > > > I'm afraid the performance drop to be hard. Mbufs are still hot from the oven at this stage, so it's not *that* expensive. I don't see a more efficient approach. > > How the port id in mbuf is used exactly? Applications that dissociate Rx itself from packet processing, or whenever a networking stack is involved. Basically every time some code wonders where a packet comes from due to lack of context and looks at m->port for the answer (e.g. checking that a packet arrives on the right port given its destination address). > > What crash are you seeing? None, thankfully. In my specific use case, 6WINDGate's stack simply drops traffic coming from unknown ports. However nothing prevents applications from using m->port as an index of some array they allocated to quickly retrieve port context without looking it up. They wouldn't expect indices they do not know about in there; assuming it will result in a crash is not far fetched. > Another way to fix it without performance drop would be to add > a new driver op to set the top-level port id. > This top-level id would be stored in the private structure of the port, > initialized with the port id of the port itself, and used to fill mbufs. > > Thoughts? Adding a new devop as a fix would be a problem for stable releases, so this patch is definitely needed, at least as a first step. I'm not against a new API, however would it be worth the trouble? Especially considering it would only be used by failsafe-like drivers with something to hide from applications which is not the main use case. For some PMDs, this operation could only be done at init time before port ID is stored in private Rx queue data for fast retrieval. Retrieving it through a pointer so it can be updated anytime would make it more expensive than necessary for them. It's understood that having failsafe in the dataplane has a cost, but even with the proposed fix, that cost is dwarfed by the amount of work done by a true PMD (and the application) for Rx processing. My suggestion is to wait for someone to complain about the performance compared to what they had before that fix, only then see what we can do. -- Adrien Mazarguil 6WIND