From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752159AbcDUTWl (ORCPT ); Thu, 21 Apr 2016 15:22:41 -0400 Received: from mail-io0-f180.google.com ([209.85.223.180]:34738 "EHLO mail-io0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751041AbcDUTWk (ORCPT ); Thu, 21 Apr 2016 15:22:40 -0400 MIME-Version: 1.0 In-Reply-To: References: <1461259276-54151-1-git-send-email-babu.moger@oracle.com> Date: Thu, 21 Apr 2016 12:22:38 -0700 Message-ID: Subject: Re: [PATCH] ixgbevf: Fix relaxed order settings in VF driver From: Alexander Duyck To: Babu Moger Cc: Jeff Kirsher , "Brandeburg, Jesse" , shannon nelson , Carolyn Wyborny , "Skidmore, Donald C" , Bruce W Allan , John Ronciak , Mitch Williams , intel-wired-lan , Netdev , "linux-kernel@vger.kernel.org" , Sowmini Varadhan Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 21, 2016 at 11:13 AM, Alexander Duyck wrote: > On Thu, Apr 21, 2016 at 10:21 AM, Babu Moger 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 > > 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. - Alex From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Date: Thu, 21 Apr 2016 12:22:38 -0700 Subject: [Intel-wired-lan] [PATCH] ixgbevf: Fix relaxed order settings in VF driver In-Reply-To: References: <1461259276-54151-1-git-send-email-babu.moger@oracle.com> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Thu, Apr 21, 2016 at 11:13 AM, Alexander Duyck wrote: > On Thu, Apr 21, 2016 at 10:21 AM, Babu Moger 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 > > 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. - Alex