DPDK patches and discussions
 help / color / mirror / Atom feed
From: Chas Williams <3chas3@gmail.com>
To: Bruce Richardson <bruce.richardson@intel.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>
Cc: Olivier Matz <olivier.matz@6wind.com>,
	dev@dpdk.org, Stephen Hemminger <stephen@networkplumber.org>,
	Chas Williams <chas3@att.com>
Subject: Re: [dpdk-dev] [PATCH] net: do not insert VLAN tag to shared mbufs
Date: Tue, 16 Apr 2019 14:32:18 -0400	[thread overview]
Message-ID: <267fabc6-ce13-a91c-8a34-401252d3b277@gmail.com> (raw)
In-Reply-To: <20190416162824.GA1886@bricha3-MOBL.ger.corp.intel.com>



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 <ferruh.yigit@intel.com>
>> ---
>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>> Cc: Chas Williams <chas3@att.com>
>>
>> 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?

> 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.

> /Bruce
> 

  parent reply	other threads:[~2019-04-16 18:32 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-16 15:51 Ferruh Yigit
2019-04-16 15:51 ` Ferruh Yigit
2019-04-16 16:28 ` Bruce Richardson
2019-04-16 16:28   ` Bruce Richardson
2019-04-16 18:32   ` Chas Williams [this message]
2019-04-16 18:32     ` Chas Williams
2019-04-17  8:12     ` Bruce Richardson
2019-04-17  8:12       ` Bruce Richardson
2019-05-13 12:43       ` Olivier Matz
2019-05-13 12:43         ` Olivier Matz
2019-07-04 14:01         ` Thomas Monjalon
2019-04-16 18:22 ` Chas Williams
2019-04-16 18:22   ` Chas Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=267fabc6-ce13-a91c-8a34-401252d3b277@gmail.com \
    --to=3chas3@gmail.com \
    --cc=bruce.richardson@intel.com \
    --cc=chas3@att.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=olivier.matz@6wind.com \
    --cc=stephen@networkplumber.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).