All of lore.kernel.org
 help / color / mirror / Atom feed
From: Robert Stonehouse <rstonehouse@solarflare.com>
To: David Miller <davem@davemloft.net>
Cc: <sshah@solarflare.com>, <netdev@vger.kernel.org>,
	<linux-net-drivers@solarflare.com>
Subject: Re: [PATCH net v4 1/2] sfc: use 64-bit writes for PIO.
Date: Thu, 5 Jun 2014 18:08:48 +0100	[thread overview]
Message-ID: <5390A420.9070903@solarflare.com> (raw)
In-Reply-To: <20140603.155807.2227032324368753328.davem@davemloft.net>

On 03/06/14 23:58, David Miller wrote:
> Did you do any functional testing of this on such a machine?
>
> I'm extremely disappointed in this patch submission, because you
> didn't even_compile_  test this in the environment where you claim
> the problem exists.

The next patch posted in the series would have meant that this change 
was limited to x86_64 systems.
I am sorry that by posting the patches in an incorrect order/not merging 
them we gave the impression that we do not test these patches (but hands 
up, patch 1/2 did break 32 bit compiles and certainly did need more 
work. Thanks to Sergei, Joe and yourself for comments)

For x86_64 machines we checked writeq() implementations in io.h and 
disassembled the module to ensure that a single 64bit access was used.

I would like to get an improved bug fix upstream, as I know this rare 
hardware issue has been hit in the wild  and with TX checksum offload 
enabled corrupted data can get to the application.

The reason for limiting to x86_64 is that in this code path there is an 
IO write to a write-combined mapping. It was a very hard case to hit 
(believed to depend on the timing of accesses and resulting 
write-combining behaviour), but we never reproduced it in our lab.
Limiting this IO to WC mappings to x86_64 only was a safety measure 
given the amount of hours of testing we have on x86_64 vs.other 64 bit 
platforms that we test with (PPC64)
This feature gives a small latency win on this hardware family for small 
packets.

Unless there are more comments to consider the next post will be a 
merged patch, addressing all points raised so far.

Regards
Rob

  reply	other threads:[~2014-06-05 17:08 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-03 10:02 [PATCH net v4 0/2] sfc: Restrict PIO for 64bit arch in order to avoid data corruption Shradha Shah
2014-06-03 10:04 ` [PATCH net v4 1/2] sfc: use 64-bit writes for PIO Shradha Shah
2014-06-03 16:45   ` Sergei Shtylyov
2014-06-03 17:55     ` Joe Perches
2014-06-03 22:58   ` David Miller
2014-06-05 17:08     ` Robert Stonehouse [this message]
2014-06-03 10:05 ` [PATCH net v4 2/2] sfc: Restrict PIO to 64-bit architectures Shradha Shah

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=5390A420.9070903@solarflare.com \
    --to=rstonehouse@solarflare.com \
    --cc=davem@davemloft.net \
    --cc=linux-net-drivers@solarflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=sshah@solarflare.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.