From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 0F38B1B553 for ; Wed, 17 Apr 2019 10:13:00 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2019 01:12:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,361,1549958400"; d="scan'208";a="132092848" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.220.103]) by orsmga007.jf.intel.com with SMTP; 17 Apr 2019 01:12:56 -0700 Received: by (sSMTP sendmail emulation); Wed, 17 Apr 2019 09:12:55 +0100 Date: Wed, 17 Apr 2019 09:12:55 +0100 From: Bruce Richardson To: Chas Williams <3chas3@gmail.com> Cc: Ferruh Yigit , Olivier Matz , dev@dpdk.org, Stephen Hemminger , Chas Williams Message-ID: <20190417081254.GA1890@bricha3-MOBL.ger.corp.intel.com> References: <20190416155126.26438-1-ferruh.yigit@intel.com> <20190416162824.GA1886@bricha3-MOBL.ger.corp.intel.com> <267fabc6-ce13-a91c-8a34-401252d3b277@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <267fabc6-ce13-a91c-8a34-401252d3b277@gmail.com> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH] net: do not insert VLAN tag to shared mbufs 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: Wed, 17 Apr 2019 08:13:01 -0000 On Tue, Apr 16, 2019 at 02:32:18PM -0400, Chas Williams wrote: > > > On 4/16/19 12:28 PM, Bruce Richardson wrote: > > On Tue, Apr 16, 2019 at 04:51:26PM +0100, Ferruh Yigit wrote: > > > The vlan_insert() is buggy when it tires to handle the shared mbufs, > > > instead don't support inserting VLAN tag into shared mbufs and return > > > an error for that case. > > > > > > Signed-off-by: Ferruh Yigit > > > --- > > > Cc: Stephen Hemminger > > > Cc: Chas Williams > > > > > > This is another approach to RFC to fix the vlan_insert: > > > https://patches.dpdk.org/patch/51870/ > > > > > > vlan_insert() mostly used by drivers to insert VLAN tag into packet > > > data in Tx path, drivers creating new copies of mbufs in Tx path may > > > result unexpected behavior, like not freed or double freed mbufs. > > > --- > > > lib/librte_net/rte_ether.h | 11 ++--------- > > > 1 file changed, 2 insertions(+), 9 deletions(-) > > > > > So what is the API to be used if one does want to insert a vlan tag into a > > shared mbuf? > > It's unlikely you would ever want to do that. Have one thread perform > some operation on the mbuf and other threads would expect this to have > happened? It seems counter to the way that packets might flow through an > application. Typically, you would insert the vlan and then share > the mbuf. Modifying a shared mbuf should make you ask, what are the > other copies expecting? > The thing is that the reference count only indicates the number of pointers to a buffer, it doesn't identify what parts are in use. So in the fragmentation case, there may only be one mbuf actually referencing the header part of the packet, with all other references to the memory being to other parts further in. However, point taken about how the app pipeline layout would probably make this issue unlikely. > > Also, why is it such a problem to create new copies of data inside the > > driver if that is necessary? You create a copy and use that, freeing the > > original (i.e. in all likelyhood decrememting the ref-count since you no > > longer use it). You already have the pointer to the mbuf pool from the > > original buffer so you can get a copy from the same place. I'm curious to > > know why it would be impossible to do a functionally correct > > implementation? > > It is not an issue to do this correctly. Hemminger did submit a patch > that appeared to do this correctly (I haven't tested it). As mentioned > earlier the tricky part is returning the buffer to the application. If > you create a copy and transmit fails, you need to free that buffer or > return it to the application for it to free. If you free the buffer when > making a buffer, you certainly can't return it to the application for > it to be freed a second time. > Right. For transmit though, in most cases the only reason for failure is lack of space in a transmit ring, so most NIC drivers can be sure of success before cloning. Overall, it seems the consensus is that for real-world cases it's better to have this patch than not, so I'm ok for it to go into DPDK. /Bruce 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 C0C4DA00E6 for ; Wed, 17 Apr 2019 10:13:03 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B69F41B5A1; Wed, 17 Apr 2019 10:13:02 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 0F38B1B553 for ; Wed, 17 Apr 2019 10:13:00 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2019 01:12:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,361,1549958400"; d="scan'208";a="132092848" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.220.103]) by orsmga007.jf.intel.com with SMTP; 17 Apr 2019 01:12:56 -0700 Received: by (sSMTP sendmail emulation); Wed, 17 Apr 2019 09:12:55 +0100 Date: Wed, 17 Apr 2019 09:12:55 +0100 From: Bruce Richardson To: Chas Williams <3chas3@gmail.com> Cc: Ferruh Yigit , Olivier Matz , dev@dpdk.org, Stephen Hemminger , Chas Williams Message-ID: <20190417081254.GA1890@bricha3-MOBL.ger.corp.intel.com> References: <20190416155126.26438-1-ferruh.yigit@intel.com> <20190416162824.GA1886@bricha3-MOBL.ger.corp.intel.com> <267fabc6-ce13-a91c-8a34-401252d3b277@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <267fabc6-ce13-a91c-8a34-401252d3b277@gmail.com> User-Agent: Mutt/1.11.4 (2019-03-13) Subject: Re: [dpdk-dev] [PATCH] net: do not insert VLAN tag to shared mbufs 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: <20190417081255.K5WOWuSShcsIO1BqR60M2I4FPGI6wUvo3W8rIk0tWgo@z> On Tue, Apr 16, 2019 at 02:32:18PM -0400, Chas Williams wrote: > > > On 4/16/19 12:28 PM, Bruce Richardson wrote: > > On Tue, Apr 16, 2019 at 04:51:26PM +0100, Ferruh Yigit wrote: > > > The vlan_insert() is buggy when it tires to handle the shared mbufs, > > > instead don't support inserting VLAN tag into shared mbufs and return > > > an error for that case. > > > > > > Signed-off-by: Ferruh Yigit > > > --- > > > Cc: Stephen Hemminger > > > Cc: Chas Williams > > > > > > This is another approach to RFC to fix the vlan_insert: > > > https://patches.dpdk.org/patch/51870/ > > > > > > vlan_insert() mostly used by drivers to insert VLAN tag into packet > > > data in Tx path, drivers creating new copies of mbufs in Tx path may > > > result unexpected behavior, like not freed or double freed mbufs. > > > --- > > > lib/librte_net/rte_ether.h | 11 ++--------- > > > 1 file changed, 2 insertions(+), 9 deletions(-) > > > > > So what is the API to be used if one does want to insert a vlan tag into a > > shared mbuf? > > It's unlikely you would ever want to do that. Have one thread perform > some operation on the mbuf and other threads would expect this to have > happened? It seems counter to the way that packets might flow through an > application. Typically, you would insert the vlan and then share > the mbuf. Modifying a shared mbuf should make you ask, what are the > other copies expecting? > The thing is that the reference count only indicates the number of pointers to a buffer, it doesn't identify what parts are in use. So in the fragmentation case, there may only be one mbuf actually referencing the header part of the packet, with all other references to the memory being to other parts further in. However, point taken about how the app pipeline layout would probably make this issue unlikely. > > Also, why is it such a problem to create new copies of data inside the > > driver if that is necessary? You create a copy and use that, freeing the > > original (i.e. in all likelyhood decrememting the ref-count since you no > > longer use it). You already have the pointer to the mbuf pool from the > > original buffer so you can get a copy from the same place. I'm curious to > > know why it would be impossible to do a functionally correct > > implementation? > > It is not an issue to do this correctly. Hemminger did submit a patch > that appeared to do this correctly (I haven't tested it). As mentioned > earlier the tricky part is returning the buffer to the application. If > you create a copy and transmit fails, you need to free that buffer or > return it to the application for it to free. If you free the buffer when > making a buffer, you certainly can't return it to the application for > it to be freed a second time. > Right. For transmit though, in most cases the only reason for failure is lack of space in a transmit ring, so most NIC drivers can be sure of success before cloning. Overall, it seems the consensus is that for real-world cases it's better to have this patch than not, so I'm ok for it to go into DPDK. /Bruce