[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <57192C7D.9080507@oracle.com>
Date: Thu, 21 Apr 2016 14:39:41 -0500
From: Babu Moger <babu.moger@...cle.com>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
shannon nelson <shannon.nelson@...el.com>,
Carolyn Wyborny <carolyn.wyborny@...el.com>,
"Skidmore, Donald C" <donald.c.skidmore@...el.com>,
Bruce W Allan <bruce.w.allan@...el.com>,
John Ronciak <john.ronciak@...el.com>,
Mitch Williams <mitch.a.williams@...el.com>,
intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
Netdev <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sowmini Varadhan <sowmini.varadhan@...cle.com>
Subject: Re: [PATCH] ixgbevf: Fix relaxed order settings in VF driver
Hi Alex,
On 4/21/2016 2:22 PM, Alexander Duyck wrote:
> On Thu, Apr 21, 2016 at 11:13 AM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Thu, Apr 21, 2016 at 10:21 AM, Babu Moger <babu.moger@...cle.com> wrote:
>>> Current code writes the tx/rx relaxed order without reading it first.
>>> This can lead to unintended consequences as we are forcibly writing
>>> other bits.
>>
>> The consequences were very much intended as there are situations where
>> enabling relaxed ordering can lead to data corruption.
>>
>>> We noticed this problem while testing VF driver on sparc. Relaxed
>>> order settings for rx queue were all messed up which was causing
>>> performance drop with VF interface.
>>
>> What additional relaxed ordering bits are you enabling on Sparc? I'm
>> assuming it is just the Rx data write back but I want to verify.
>>
>>> Fixed it by reading the registers first and setting the specific
>>> bit of interest. With this change we are able to match the bandwidth
>>> equivalent to PF interface.
>>>
>>> Signed-off-by: Babu Moger <babu.moger@...cle.com>
>>
>> Fixed is a relative term here since you are only chasing performance
>> from what I can tell. We need to make certain that this doesn't break
>> the driver on any other architectures by leading to things like data
>> corruption.
>>
>> - Alex
>
> It occurs to me that what might be easier is instead of altering the
> configuration on all architectures you could instead wrap the write so
> that on SPARC you include the extra bits you need and on all other
> architectures you leave the write as-is similar to how the code in the
> ixgbe_start_hw_gen2 only clears the bits if CONFIG_SPARC is not
> defined.
Here are the default values that I see when testing on Sparc.
Default tx value 0x2a00
All below 3 set
#define IXGBE_DCA_TXCTRL_DESC_RRO_EN (1 << 9) /* Tx rd Desc Relax Order */
#define IXGBE_DCA_TXCTRL_DESC_WRO_EN (1 << 11) /* Tx Desc writeback RO bit */
#define IXGBE_DCA_TXCTRL_DATA_RRO_EN (1 << 13) /* Tx rd data Relax Order */
I am not too worried about tx values. I can keep it as it is. It did not
seem to cause any problems right now.
Default rx value 0xb200
All below 3 set plus one more
#define IXGBE_DCA_RXCTRL_DESC_RRO_EN (1 << 9) /* DCA Rx rd Desc Relax Order */
#define IXGBE_DCA_RXCTRL_DATA_WRO_EN (1 << 13) /* Rx wr data Relax Order */
#define IXGBE_DCA_RXCTRL_HEAD_WRO_EN (1 << 15) /* Rx wr header RO */
Is there a reason to disable IXGBE_DCA_RXCTRL_DATA_WRO_EN and
IXGBE_DCA_RXCTRL_HEAD_WRO_EN for RX?
I would think CONFIG_SPARC should be our last option. What do you think?
>
> - Alex
>
Powered by blists - more mailing lists