From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f53.google.com (mail-wg0-f53.google.com [74.125.82.53]) by dpdk.org (Postfix) with ESMTP id 6E96A5A4B for ; Thu, 5 Mar 2015 10:36:18 +0100 (CET) Received: by wghk14 with SMTP id k14so8309295wgh.7 for ; Thu, 05 Mar 2015 01:36:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=jMoSXQAnsyKDntppCxDaPLgXNN/uo7qQg6kuTsezo+w=; b=jKfFrdpJuPuH5mKEnPB31I60856citHh7ePon3wQAlANt+kAmyTFRpjHCo2BbW/McB ngDaY+R/ASwFeWdnj/Bd2GGu8+V1IhQ5aplkoid5up4velhFKrnsx/Glh23VWvUbBDL/ /LQ1QCRUEcpIRVnfBDNb68WRiiQCdoTxxptOFn3LTZeQp1EZicr9xTDlJoD4BpFX+qc+ pUHfNSNTcoD9npkeRnO+at1RzOzzcg9xpY+BkfEUxxBTMOngOcV95plQA2hx8Aq2Ggl4 Idws2F58HYTqxC3mwi0IcM37elriuayxJVX6kAQFkpPMe6HRe/e7mJZVtwdXfmnzUjGH TwIQ== X-Gm-Message-State: ALoCoQkFFiys3ycd1W7m2Pa90s5Ay/TTegU2LSh/KQH39/p+eNowKr63fwo6cJQs+ikp1LzkeV14 X-Received: by 10.195.13.168 with SMTP id ez8mr16637636wjd.30.1425548178243; Thu, 05 Mar 2015 01:36:18 -0800 (PST) Received: from [10.0.0.166] ([212.143.139.214]) by mx.google.com with ESMTPSA id lx10sm9666813wjb.17.2015.03.05.01.36.17 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Mar 2015 01:36:17 -0800 (PST) Message-ID: <54F82390.9090500@cloudius-systems.com> Date: Thu, 05 Mar 2015 11:36:16 +0200 From: Vlad Zolotarov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Stephen Hemminger 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> <20150304105424.3a789782@urahara> In-Reply-To: <20150304105424.3a789782@urahara> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Thu, 05 Mar 2015 09:36:18 -0000 On 03/04/15 20:54, Stephen Hemminger wrote: > 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); This workaround doesn't address the described above issue - it just hides it inside a macro, which is even uglier. The main reason I haven't fixed this issue in (at least) a function I've added is that the hw->rx_ring (HW ring) is defined as volatile and this fact is used all over the file in different places and all such places have to be fixed if I drop the "volatile" qualifier which should be the first thing to do. > > > >