LinuxLists.cc - [ 026/171 ] sfc: Only use TX push if a single descriptor is to be written

2013-04-11 20:26:23

Subject: [ 026/171 ] sfc: Only use TX push if a single descriptor is to be written

3.6.11.2 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Ben Hutchings <[email protected]>

[ Upstream commit fae8563b25f73dc584a07bcda7a82750ff4f7672 ]

Using TX push when notifying the NIC of multiple new descriptors in
the ring will very occasionally cause the TX DMA engine to re-use an
old descriptor. This can result in a duplicated or partly duplicated
packet (new headers with old data), or an IOMMU page fault. This does
not happen when the pushed descriptor is the only one written.

TX push also provides little latency benefit when a packet requires
more than one descriptor.

Signed-off-by: Ben Hutchings <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
drivers/net/ethernet/sfc/nic.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/nic.c b/drivers/net/ethernet/sfc/nic.c
index 326d799..a1c3d80 100644
--- a/drivers/net/ethernet/sfc/nic.c
+++ b/drivers/net/ethernet/sfc/nic.c
@@ -377,7 +377,8 @@ efx_may_push_tx_desc(struct efx_tx_queue *tx_queue, unsigned int write_count)
return false;

tx_queue->empty_read_count = 0;
- return ((empty_read_count ^ write_count) & ~EFX_EMPTY_COUNT_VALID) == 0;
+ return ((empty_read_count ^ write_count) & ~EFX_EMPTY_COUNT_VALID) == 0
+ && tx_queue->write_count - write_count == 1;
}

/* For each entry inserted into the software descriptor ring, create a
--
1.7.10.4

2013-04-11 21:16:42

by Ben Hutchings

[permalink] [raw]

Subject: Re: [ 026/171 ] sfc: Only use TX push if a single descriptor is to be written

Aside from #21-26 in this series, and the deadlock fix required on top
of #24, there are several more fixes for sfc that I think are suitable
for 3.6.11.y.

These commits were cherry-picked for 3.4.38 and can also be
cherry-picked cleanly on top of 3.6.11.1 plus the 7 patches you already
have:

d5e8cc6c946e sfc: Really disable flow control while flushing
bfeed902946a sfc: Convert firmware subtypes to native byte order in efx_mcdi_get_board_cfg()
9724a8504c87 sfc: Add parentheses around use of bitfield macro arguments
0a6e5008a9df sfc: Fix MCDI structure field lookup
450783747f42 sfc: Avoid generating over-length MC_CMD_FLUSH_RX_QUEUES request
525d9e824018 sfc: Work-around flush timeout when flushes have completed
ef492f11efed sfc: Correctly initialise reset_method in siena_test_chip()
ebf98e797b4e sfc: Fix timekeeping in efx_mcdi_poll()

Please let me know whether you're prepared to include these in the
current update. I can then run some automated tests with your selected
set of patches applied.

Ben.

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

2013-04-11 21:22:18

by Steven Rostedt

[permalink] [raw]

Subject: Re: [ 026/171 ] sfc: Only use TX push if a single descriptor is to be written

2013-04-12 22:05:37

by Ben Hutchings

[permalink] [raw]

Subject: Re: [ 026/171 ] sfc: Only use TX push if a single descriptor is to be written

On Thu, 2013-04-11 at 22:15 +0100, Ben Hutchings wrote:
> Aside from #21-26 in this series, and the deadlock fix required on top
> of #24, there are several more fixes for sfc that I think are suitable
> for 3.6.11.y.
>
> These commits were cherry-picked for 3.4.38 and can also be
> cherry-picked cleanly on top of 3.6.11.1 plus the 7 patches you already
> have:
>
> d5e8cc6c946e sfc: Really disable flow control while flushing
> bfeed902946a sfc: Convert firmware subtypes to native byte order in efx_mcdi_get_board_cfg()
> 9724a8504c87 sfc: Add parentheses around use of bitfield macro arguments
> 0a6e5008a9df sfc: Fix MCDI structure field lookup
> 450783747f42 sfc: Avoid generating over-length MC_CMD_FLUSH_RX_QUEUES request
> 525d9e824018 sfc: Work-around flush timeout when flushes have completed
> ef492f11efed sfc: Correctly initialise reset_method in siena_test_chip()
> ebf98e797b4e sfc: Fix timekeeping in efx_mcdi_poll()
>
> Please let me know whether you're prepared to include these in the
> current update. I can then run some automated tests with your selected
> set of patches applied.

The test suite found a regression which I'd forgotten about. It
was introduced in 3.6 by commit b7f514af7d6f 'sfc: Fix interface
statistics running backward' and fixed in 3.8 by commit 876be083b669
'sfc: Reset driver's MAC stats after MC reboot seen'.

That latter fix is, again, a clean cherry-pick onto 3.6.y. I don't
think I'm going to be able to re-test with this but it's sufficiently
low-risk that I'd be happy for you to add it anyway.

Ben.

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

2013-04-13 01:12:10

by Steven Rostedt

[permalink] [raw]

Subject: Re: [ 026/171 ] sfc: Only use TX push if a single descriptor is to be written

On Fri, 2013-04-12 at 23:05 +0100, Ben Hutchings wrote:
> On Thu, 2013-04-11 at 22:15 +0100, Ben Hutchings wrote:
> > Aside from #21-26 in this series, and the deadlock fix required on top
> > of #24, there are several more fixes for sfc that I think are suitable
> > for 3.6.11.y.
> >
> > These commits were cherry-picked for 3.4.38 and can also be
> > cherry-picked cleanly on top of 3.6.11.1 plus the 7 patches you already
> > have:
> >
> > d5e8cc6c946e sfc: Really disable flow control while flushing
> > bfeed902946a sfc: Convert firmware subtypes to native byte order in efx_mcdi_get_board_cfg()
> > 9724a8504c87 sfc: Add parentheses around use of bitfield macro arguments
> > 0a6e5008a9df sfc: Fix MCDI structure field lookup
> > 450783747f42 sfc: Avoid generating over-length MC_CMD_FLUSH_RX_QUEUES request
> > 525d9e824018 sfc: Work-around flush timeout when flushes have completed
> > ef492f11efed sfc: Correctly initialise reset_method in siena_test_chip()
> > ebf98e797b4e sfc: Fix timekeeping in efx_mcdi_poll()
> >
> > Please let me know whether you're prepared to include these in the
> > current update. I can then run some automated tests with your selected
> > set of patches applied.
>
> The test suite found a regression which I'd forgotten about. It
> was introduced in 3.6 by commit b7f514af7d6f 'sfc: Fix interface
> statistics running backward' and fixed in 3.8 by commit 876be083b669
> 'sfc: Reset driver's MAC stats after MC reboot seen'.
>
> That latter fix is, again, a clean cherry-pick onto 3.6.y. I don't
> think I'm going to be able to re-test with this but it's sufficiently
> low-risk that I'd be happy for you to add it anyway.

Thanks!

I included it, and will run some simple tests. If everything works, I'll
just keep it without another spamming of the mailing lists.

I wont post till after my 3.6.11.2-rt tests passes.

-- Steve