netdev - RE: [PATCH net-next 1/3] gianfar: Map head TxBD first to skip a wmb()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB4PR04MB3799AF95B8C7FB8C949476496A30@DB4PR04MB379.eurprd04.prod.outlook.com>
Date:	Mon, 22 Feb 2016 10:14:37 +0000
From:	Claudiu Manoil <claudiu.manoil@....com>
To:	David Miller <davem@...emloft.net>,
	"claudiu.manoil@...escale.com" <claudiu.manoil@...escale.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next 1/3] gianfar: Map head TxBD first to skip a wmb()

>-----Original Message-----
>From: David Miller [mailto:davem@...emloft.net]
>Sent: Monday, February 22, 2016 4:59 AM
>To: claudiu.manoil@...escale.com
>Cc: netdev@...r.kernel.org
>Subject: Re: [PATCH net-next 1/3] gianfar: Map head TxBD first to skip a
>wmb()
>
>From: Claudiu Manoil <claudiu.manoil@...escale.com>
>Date: Fri, 19 Feb 2016 12:47:32 +0200
>
>> The main point of this change is to update the buffer pointer of
>> the first BD way before the status field is updated, so that a
>> wmb() between these two operations is no longer needed.
>
>This is the completely bogus.
>
>The distance between two memory operations has no bearing upon
>whether a memory barrier is necessary between them or not.
>
>There could be 1,000 loads and stores between two dependant
>operations, you still need the memory barrier for full correctness
>if the ordering between them is important.
>
>I am not applying this patch series because it's is doing things
>and stating things which are entirely incorrect.
>

I think it can be proven that the wmb() is not needed in this case.
If it weren't so, then all the other eth drivers would require a wmb()
in their xmit(), between buffer address update and status update
for the last descriptor of a frame.
Let's take a random driver - igb, there is no wmb() between:
tx_desc->read.buffer_addr = cpu_to_le64(dma);
and
/* write last descriptor with RS and EOP bits */
cmd_type |= size | IGB_TXD_DCMD;
tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type);

Anyway, the patch series is far from being "entirely incorrect".
Even with the contested gfar_wmb() statement left in place,
the proposed code refactoring of xmit() improves execution
time, and code maintainability.