From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ie0-f182.google.com (mail-ie0-f182.google.com [209.85.223.182]) by dpdk.org (Postfix) with ESMTP id A33945A7A for ; Wed, 4 Mar 2015 19:54:23 +0100 (CET) Received: by iecar1 with SMTP id ar1so69716027iec.11 for ; Wed, 04 Mar 2015 10:54:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type:content-transfer-encoding; bh=EIgs3f6sF2ffmVfecRXdvd5N5ULOxv6L2EOzt212a1k=; b=agj+uRBIK6HdlbSGBAJc2nSBpiz9HpizzMF1473lC8p0EZhCrcn77u36R+bS9LnJA7 72pn9puphjxoD4plqvD1iv4Yb8Jnx6OpuZLeJ8SP8p/4O68zkEEhwz3zZ8quLnxvxece lX0yGZDoiRgly4/SPDCheCIM1uZmsTcrnbv0r4GsPcc21XcIWpaMNWNtZZORCGP5WkW0 VnpBk/lcQxbiWt+rvqR8qjrNQwvDxtMQqX5E/lMtcQj6uDxZchF5CR4kMGio5Q4zEn4y dkRRfPhJwlWFSxHeduX4xbvPu31v3RkgvsCV/yi64bTPh4rXTBibEsFaOBfINr1nJETu KfoQ== X-Gm-Message-State: ALoCoQlt+gSZ2k/yyNoeVMiEjK0Qw4JJNwGznirRuOaV8CNalzDr0APHogTLtjZ5v4CZNW8Nw8MQ X-Received: by 10.50.111.168 with SMTP id ij8mr14068402igb.43.1425495263128; Wed, 04 Mar 2015 10:54:23 -0800 (PST) Received: from urahara (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by mx.google.com with ESMTPSA id x10sm3383470igl.13.2015.03.04.10.54.22 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Mar 2015 10:54:23 -0800 (PST) Date: Wed, 4 Mar 2015 10:54:24 -0800 From: Stephen Hemminger To: Vlad Zolotarov Message-ID: <20150304105424.3a789782@urahara> In-Reply-To: <54F6BAE4.8020102@cloudius-systems.com> References: <1425412123-5227-1-git-send-email-vladz@cloudius-systems.com> <1425412123-5227-6-git-send-email-vladz@cloudius-systems.com> <20150303163359.2975c702@urahara> <54F6BAE4.8020102@cloudius-systems.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Mar 2015 18:54:24 -0000 On Wed, 04 Mar 2015 09:57:24 +0200 Vlad Zolotarov wrote: > > > On 03/04/15 02:33, Stephen Hemminger wrote: > > On Tue, 3 Mar 2015 21:48:43 +0200 > > Vlad Zolotarov 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);