netdev - Re: [PATCH net-next V1 09/10] net/mlx5: Fix global UAR mapping

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] [day] [month] [year] [list]

Date:	Mon, 29 Feb 2016 21:41:27 +0200
From:	Saeed Mahameed <saeedm@....mellanox.co.il>
To:	David Miller <davem@...emloft.net>
Cc:	Saeed Mahameed <saeedm@...lanox.com>,
	Linux Netdev List <netdev@...r.kernel.org>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Eran Ben Elisha <eranbe@...lanox.com>,
	Tal Alon <talal@...lanox.com>,
	Majd Dibbiny <majd@...lanox.com>, moshel@...lanox.com
Subject: Re: [PATCH net-next V1 09/10] net/mlx5: Fix global UAR mapping

>
> Well anyone can see that from the code.
>
> You have to explain why.

In a simple words as partially explained in the commit message we want
to have both mappings (NC and WC) available so upper layer can decide
which to choose e.g. for SQs/QPs in some cases (Small Packets) and
only when WC is supported we would like to write TX descriptors (WQEs)
using ConnectX BlueFlame feature via WC mapping and if WC is not
supported the TX descriptors would be posted in the usual way
(doorbell) via NC mapping.
this would give a latency boost for small packets.

The problem is when posting BlueFlame buffers when the mapping is not
WC i.e via NC mapping the latency will get worst than writing using
the usual way (doorbell).

so this is why we use ARCH_HAS_IOREMAP_WC to give a hint to upper
layer whether to use BlueFlame writes (WC) or doorbell writes (NC).

>
> And BTW, ARCH_HAS_IOREMAP_WC doesn't even tell you if the platform
> will actually give you a write-combining mapping.

We did some research after your comment and we are considering
removing ARCH_HAS_IOREMAP_WC from the code, we will update the patches
soon.

>
> So if it's the driver operates properly if a non-WC mapping is used
> for uar->bf_map, then get rid of this CPP test altogether PLEASE!
>
> Otherwise your driver is buggy, because ARCH_HAS_IOREMAP_WC only says
> whether the default implementation of ioremap_wc() needs to be
> provided by include/asm-generic/iomap.h It does not guarantee that a
> write-combining mapping will be provided.
>
> I really can't think of any reason why you absolutely require a
> WC mapping, and the CPP test just makes your driver look more
> ugly than it needs to me.

WC mapping is required in order to know if BlueFlame writes would give
a better latency or not.

>
> So can you please explain what the hell is happening here and why you
> are doing things this way rather than just reading the code to me?

I hope the above explains what we are trying to do here, I know it is
not perfect, but as you know the kernel IO mapping API doesn't tell if
the WC mapping was successful or not, so we used the CPP test.
but after your comment we understood it is not perfect, and we are
looking into it.

Thanks