DPDK patches and discussions
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: Vlad Zolotarov <vladz@cloudius-systems.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support
Date: Wed, 4 Mar 2015 10:54:24 -0800	[thread overview]
Message-ID: <20150304105424.3a789782@urahara> (raw)
In-Reply-To: <54F6BAE4.8020102@cloudius-systems.com>

On Wed, 04 Mar 2015 09:57:24 +0200
Vlad Zolotarov <vladz@cloudius-systems.com> wrote:

> 
> 
> On 03/04/15 02:33, Stephen Hemminger wrote:
> > On Tue,  3 Mar 2015 21:48:43 +0200
> > Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
> >
> >> +	next_desc:
> >> +		/*
> >> +		 * The code in this whole file uses the volatile pointer to
> >> +		 * ensure the read ordering of the status and the rest of the
> >> +		 * descriptor fields (on the compiler level only!!!). This is so
> >> +		 * UGLY - why not to just use the compiler barrier instead? DPDK
> >> +		 * even has the rte_compiler_barrier() for that.
> >> +		 *
> >> +		 * But most importantly this is just wrong because this doesn't
> >> +		 * ensure memory ordering in a general case at all. For
> >> +		 * instance, DPDK is supposed to work on Power CPUs where
> >> +		 * compiler barrier may just not be enough!
> >> +		 *
> >> +		 * I tried to write only this function properly to have a
> >> +		 * starting point (as a part of an LRO/RSC series) but the
> >> +		 * compiler cursed at me when I tried to cast away the
> >> +		 * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm
> >> +		 * keeping it the way it is for now.
> >> +		 *
> >> +		 * The code in this file is broken in so many other places and
> >> +		 * will just not work on a big endian CPU anyway therefore the
> >> +		 * lines below will have to be revisited together with the rest
> >> +		 * of the ixgbe PMD.
> >> +		 *
> >> +		 * TODO:
> >> +		 *    - Get rid of "volatile" crap and let the compiler do its
> >> +		 *      job.
> >> +		 *    - Use the proper memory barrier (rte_rmb()) to ensure the
> >> +		 *      memory ordering below.
> > This comment screams "this is broken".
> > Why not get proper architecture independent barriers in DPDK first.
> 
> This series is orthogonal to the issue above. I just couldn't stand to 
> mention this ugliness when I noticed it on the way.
> Note that although this is obviously not the right way to write this 
> kind of code it is still not a bug and most likely the performance 
> implications are minimal here.
> The only overhead is that there may be read "too much" data from the 
> descriptor that we may not actually need. The descriptor is 16 bytes so 
> this doesn't seem to be a critical issue.
> 
> So, fixing the above issue may wait, especially since the same s..t may 
> be found in other Intel PMDs (see i40e for example). Fixing this issue 
> should be a matter of a massive cleanup series that cover all the 
> relevant PMDs. Of course we may start with ixgbe but even in this single 
> PMD there are at least 3 non-LRO related functions that have to be 
> fixed, so IMHO even fixing ONLY ixgbe should be a matter of a separate 
> series.

In userspace-rcu and kernel there is a simple macro that would make this
kind of code more sane.

What about adding:

#define rte_access_once(x)  (*(volatile typeof(x) *)&(x))

Then doing
            rxdp = rte_access_once(rx_ring + idx);

  reply	other threads:[~2015-03-04 18:54 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-03 19:48 [dpdk-dev] [PATCH v1 0/5]: Add LRO support to ixgbe PMD Vlad Zolotarov
2015-03-03 19:48 ` [dpdk-dev] [PATCH v1 1/5] ixgbe: Cleanups Vlad Zolotarov
2015-03-03 19:48 ` [dpdk-dev] [PATCH v1 2/5] ixgbe: Bug fix: Properly configure Rx CRC stripping for x540 devices Vlad Zolotarov
2015-03-03 19:48 ` [dpdk-dev] [PATCH v1 3/5] ixgbe: Code refactoring Vlad Zolotarov
2015-03-03 19:48 ` [dpdk-dev] [PATCH v1 4/5] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option Vlad Zolotarov
2015-03-03 19:48 ` [dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support Vlad Zolotarov
2015-03-04  0:33   ` Stephen Hemminger
2015-03-04  7:24     ` Vlad Zolotarov
2015-03-04  0:33   ` Stephen Hemminger
2015-03-04  7:57     ` Vlad Zolotarov
2015-03-04 18:54       ` Stephen Hemminger [this message]
2015-03-05  9:36         ` Vlad Zolotarov
2015-03-04  8:05     ` Avi Kivity
2015-03-04  0:34   ` Stephen Hemminger
2015-03-04  7:57     ` Vlad Zolotarov
2015-03-04  0:36   ` Stephen Hemminger
2015-03-04  7:59     ` Vlad Zolotarov
2015-03-04 18:51       ` Stephen Hemminger

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=20150304105424.3a789782@urahara \
    --to=stephen@networkplumber.org \
    --cc=dev@dpdk.org \
    --cc=vladz@cloudius-systems.com \
    /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).