From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by dpdk.org (Postfix) with ESMTP id F408C1BC08; Thu, 18 Apr 2019 18:54:25 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 99F99254C9; Thu, 18 Apr 2019 12:54:25 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Thu, 18 Apr 2019 12:54:25 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=HPltJ1bT2iWG0l/1ccKnM5wAO6FjlrL8p4zMHJklDow=; b=YouQi9K2hoP3 banFsAujDKcpyB2wr1c5zt83KdjP5pjRRjc0xtigqqGsVLlJvlkIjdWqlMrgAw9m w5SmdhOiwhNq7l+l5HOXc1SU7QHqrfSn83AJUCFHLFsLc+sON3xsHXU+IfV+vaTA iywhv2kF2JfjyXfXN+d3o1SE5odRpGs= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=HPltJ1bT2iWG0l/1ccKnM5wAO6FjlrL8p4zMHJklD ow=; b=Fqnli80AjmBMTiShfLbrRQ7UV19aDdW6S87zVLrHkWr14aLmYJobKfhie rMuqfjBGm7Yv/HhtRbd6AsLbwX1rj30/b/KtloTFtkZEX6zlDZNK+NvBtHPhStSN fKkMfy3hcRwYJ/pjifXDZXTOzBrfJMPmMjnPjoQNK7wRk7vGWKZ9v0E4vVAEL6B1 N69cCAmhgIo1eEjCNavzO7u9JH6EKqVL4w6lbEyBrekPhpphMBWw5SU8JNl7AvvH iuuXkMUBa8nbMg3l4vjf/eVuzgEMFFRCKV8OJED/MtTazbqNeWGNp4Hzf2gZ/ghg ATaYO08corRNCVIZ/schzAp6JlaEg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduuddrfeehgdegiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvghtnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id C08A4E4383; Thu, 18 Apr 2019 12:54:23 -0400 (EDT) From: Thomas Monjalon To: Adrien Mazarguil Cc: dev@dpdk.org, Gaetan Rivet , Ferruh Yigit , David Marchand , stable@dpdk.org, Ali Alnubani Date: Thu, 18 Apr 2019 18:54:22 +0200 Message-ID: <1609324.Zy8sxAcIVK@xps> In-Reply-To: <20190418164624.GG4889@6wind.com> References: <20190418130419.25675-1-adrien.mazarguil@6wind.com> <2732210.eFvFzSlaYO@xps> <20190418164624.GG4889@6wind.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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: , X-List-Received-Date: Thu, 18 Apr 2019 16:54:26 -0000 18/04/2019 18:46, Adrien Mazarguil: > 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? Yes please, except if Ferruh is already doing the change on apply. > > > > +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. Yes, Ali did some quick tests showing no perf drop. > > > 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. I don't understand this comment. The port id is currently retrieved via some pointers already. I suggest to look at private structure, it is not different. > 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. OK 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 4F8E2A00E6 for ; Thu, 18 Apr 2019 18:54:28 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A99F51BC10; Thu, 18 Apr 2019 18:54:26 +0200 (CEST) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by dpdk.org (Postfix) with ESMTP id F408C1BC08; Thu, 18 Apr 2019 18:54:25 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 99F99254C9; Thu, 18 Apr 2019 12:54:25 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Thu, 18 Apr 2019 12:54:25 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=HPltJ1bT2iWG0l/1ccKnM5wAO6FjlrL8p4zMHJklDow=; b=YouQi9K2hoP3 banFsAujDKcpyB2wr1c5zt83KdjP5pjRRjc0xtigqqGsVLlJvlkIjdWqlMrgAw9m w5SmdhOiwhNq7l+l5HOXc1SU7QHqrfSn83AJUCFHLFsLc+sON3xsHXU+IfV+vaTA iywhv2kF2JfjyXfXN+d3o1SE5odRpGs= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=HPltJ1bT2iWG0l/1ccKnM5wAO6FjlrL8p4zMHJklD ow=; b=Fqnli80AjmBMTiShfLbrRQ7UV19aDdW6S87zVLrHkWr14aLmYJobKfhie rMuqfjBGm7Yv/HhtRbd6AsLbwX1rj30/b/KtloTFtkZEX6zlDZNK+NvBtHPhStSN fKkMfy3hcRwYJ/pjifXDZXTOzBrfJMPmMjnPjoQNK7wRk7vGWKZ9v0E4vVAEL6B1 N69cCAmhgIo1eEjCNavzO7u9JH6EKqVL4w6lbEyBrekPhpphMBWw5SU8JNl7AvvH iuuXkMUBa8nbMg3l4vjf/eVuzgEMFFRCKV8OJED/MtTazbqNeWGNp4Hzf2gZ/ghg ATaYO08corRNCVIZ/schzAp6JlaEg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduuddrfeehgdegiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvghtnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id C08A4E4383; Thu, 18 Apr 2019 12:54:23 -0400 (EDT) From: Thomas Monjalon To: Adrien Mazarguil Cc: dev@dpdk.org, Gaetan Rivet , Ferruh Yigit , David Marchand , stable@dpdk.org, Ali Alnubani Date: Thu, 18 Apr 2019 18:54:22 +0200 Message-ID: <1609324.Zy8sxAcIVK@xps> In-Reply-To: <20190418164624.GG4889@6wind.com> References: <20190418130419.25675-1-adrien.mazarguil@6wind.com> <2732210.eFvFzSlaYO@xps> <20190418164624.GG4889@6wind.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="UTF-8" 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: <20190418165422.QWS_Vq94PmkcBKzRPwYKZ5ESWA8eAziElZicIPVDp7k@z> 18/04/2019 18:46, Adrien Mazarguil: > 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? Yes please, except if Ferruh is already doing the change on apply. > > > > +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. Yes, Ali did some quick tests showing no perf drop. > > > 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. I don't understand this comment. The port id is currently retrieved via some pointers already. I suggest to look at private structure, it is not different. > 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. OK