Re: [PATCH 0/4] support for write combining

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Thomas,

I have a question about support for write combining patch set.
It got ack from Bruce Richardson more then month ago.
Also no one has any further comments about it.
What is the next step to commit them to DPDK source?

Best regards,
Rafal Kozik

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Thomas,

I would like to kindly remind about question about support for write
combining patch set:
https://mails.dpdk.org/archives/dev/2018-April/096749.html

It got ack from Bruce Richardson,
what is the next step to commit them to DPDK source?

Best regards,
Rafal Kozik


2018-06-11 11:32 GMT+02:00 Rafał Kozik <rk@semihalf.com>:
> Hello Thomas,
>
> I have a question about support for write combining patch set.
> It got ack from Bruce Richardson more then month ago.
> Also no one has any further comments about it.
> What is the next step to commit them to DPDK source?
>
> Best regards,
> Rafal Kozik

[PATCH 0/4] support for write combining

[Rafal Kozik]

Support for write combining.

Rafal Kozik (4):
  igb_uio: add wc option
  bus/pci: reference driver structure
  eal: enable WC during resources mapping
  net/ena: enable WC

 drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
 drivers/bus/pci/pci_common.c    | 13 ++++++++-----
 drivers/bus/pci/rte_bus_pci.h   |  2 ++
 drivers/net/ena/ena_ethdev.c    |  3 ++-
 kernel/linux/igb_uio/igb_uio.c  | 17 ++++++++++++++---
 5 files changed, 54 insertions(+), 20 deletions(-)

-- 
2.7.4

Re: [PATCH 0/4] support for write combining

[Bruce Richardson]

On Wed, Apr 11, 2018 at 04:07:13PM +0200, Rafal Kozik wrote:
> Support for write combining.
> 
> Rafal Kozik (4):
>   igb_uio: add wc option
>   bus/pci: reference driver structure
>   eal: enable WC during resources mapping
>   net/ena: enable WC
> 
>  drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
>  drivers/bus/pci/pci_common.c    | 13 ++++++++-----
>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>  drivers/net/ena/ena_ethdev.c    |  3 ++-
>  kernel/linux/igb_uio/igb_uio.c  | 17 ++++++++++++++---
>  5 files changed, 54 insertions(+), 20 deletions(-)
> 
Couple of thoughts on this set.

You add an option to the kernel module to allow wc to be supported on a
device, but when we go to do PCI mapping, we either map the regular
resource file or the _wc one. Therefore:

1. Why not always have igb_uio support write-combining since it can be
controlled thereafter via userspace mapping one file or another?

2. Why not always map both resource and resource_wc files at the PCI level,
and make them available via different pointers to the driver? Then the
driver can choose at the per-access level which it wants to use. For
example, for init of a device, a driver may do all register access via
uncachable memory, and only use the write-combined support for
performance-critical parts. [I have a draft patch lying around here
somewhere that does something similar to that.]

One last question - if using vfio-pci kernel module, do the resource_wc
files present the bars as write-combined memory type, or are they
uncachable?

Regards,
/Bruce

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Bruce,

thank you for your advices.

> 1. Why not always have igb_uio support write-combining since it can be
> controlled thereafter via userspace mapping one file or another?

I added parameter to the igb_uio because currently it perform ioremap
and fails if it return NULL.
But performing ioremap makes it impossible to use WC, so I remove it.
ENA driver work well after this change, but I cannot test it on all
drivers and all platforms.
It seems to me that making it configurable prevents form spoiling
other drivers that could use internal_addr returned by ioremap.

> 2. Why not always map both resource and resource_wc files at the PCI level,
> and make them available via different pointers to the driver? Then the
> driver can choose at the per-access level which it wants to use. For
> example, for init of a device, a driver may do all register access via
> uncachable memory, and only use the write-combined support for
> performance-critical parts. [I have a draft patch lying around here
> somewhere that does something similar to that.]

I tried to implement this idea but without good results. I get mapping
with or without WC depending on mapping order.
As I was trying to find solution I come across with this paper:
https://www.kernel.org/doc/ols/2008/ols2008v2-pages-135-144.pdf
In section 5.3 and 5.4 it is discussing access to PCI resources.
According to it:

A request to uncached access can fail if there is already
an existing write-combine mapping for that region. A
request for write-combine access can succeed with un-
cached mapping instead, in the case of already existing
uncached mapping for this region.

We cannot use WC all the time, because it not guaranteed writing order.
On this basis I suppose that better option is to map each resource
only once depending on parameter provided by PMD.

> One last question - if using vfio-pci kernel module, do the resource_wc
> files present the bars as write-combined memory type, or are they
> uncachable?

I tried to use VFIO to map WC memory, but without success.

Best regards,
Rafal Kozik

2018-04-11 16:42 GMT+02:00 Bruce Richardson <bruce.richardson@intel.com>:
> On Wed, Apr 11, 2018 at 04:07:13PM +0200, Rafal Kozik wrote:
>> Support for write combining.
>>
>> Rafal Kozik (4):
>>   igb_uio: add wc option
>>   bus/pci: reference driver structure
>>   eal: enable WC during resources mapping
>>   net/ena: enable WC
>>
>>  drivers/bus/pci/linux/pci_uio.c | 39 ++++++++++++++++++++++++++++-----------
>>  drivers/bus/pci/pci_common.c    | 13 ++++++++-----
>>  drivers/bus/pci/rte_bus_pci.h   |  2 ++
>>  drivers/net/ena/ena_ethdev.c    |  3 ++-
>>  kernel/linux/igb_uio/igb_uio.c  | 17 ++++++++++++++---
>>  5 files changed, 54 insertions(+), 20 deletions(-)
>>
> Couple of thoughts on this set.
>
> You add an option to the kernel module to allow wc to be supported on a
> device, but when we go to do PCI mapping, we either map the regular
> resource file or the _wc one. Therefore:
>
> 1. Why not always have igb_uio support write-combining since it can be
> controlled thereafter via userspace mapping one file or another?
>
> 2. Why not always map both resource and resource_wc files at the PCI level,
> and make them available via different pointers to the driver? Then the
> driver can choose at the per-access level which it wants to use. For
> example, for init of a device, a driver may do all register access via
> uncachable memory, and only use the write-combined support for
> performance-critical parts. [I have a draft patch lying around here
> somewhere that does something similar to that.]
>
> One last question - if using vfio-pci kernel module, do the resource_wc
> files present the bars as write-combined memory type, or are they
> uncachable?
>
> Regards,
> /Bruce

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Bruce,

As from my last post passed ten days I would kindly ask if there are
any more comments?

Best regards,
Rafal Kozik

Re: [PATCH 0/4] support for write combining

[Bruce Richardson]

On Fri, Apr 27, 2018 at 10:27:02AM +0200, Rafał Kozik wrote:
> Hello Bruce,
> 
> As from my last post passed ten days I would kindly ask if there are
> any more comments?
> 
> Best regards,
> Rafal Kozik

Hi Rafal,

sorry for not responding before. The set makes a lot more sense to me now.
However, just one more question? If igb_uio is passed the flag to make
mappings wc, what happens if you have a mix of drivers that support or
don't support wc at the driver, not the hardware level. For example,
support there is one ena adapter and one i40e adapter. The hardware on both
should be mappable as WC, but one driver can use that, the other can't. How
is it handled?

/Bruce

Re: [PATCH 0/4] support for write combining

[Rafał Kozik]

Hello Bruce,

It should work because decision about kind of mapping is made in patch
3 based on PMD request.

ENA use only one BAR in wc mode and two other without caching,
therefore not making remap in igb_uio rather not spoil anything.

I added patch 1 because this variable is provided also outside the
DPDK to Linux generic functions. I cannot test it with all drivers,
therefore I make it configurable.

But general combination of wc and not wc PMD should work, because of patch 3.
Also if WC is enabled by PMD (like in patch 4) not all BARs will by
mapped, but only those which has prefetchable flag enabled.

Best regards,
Rafal Kozik

Re: [PATCH 0/4] support for write combining

[Bruce Richardson]

On Mon, Apr 30, 2018 at 10:07:07AM +0200, Rafał Kozik wrote:
> Hello Bruce,
> 
> It should work because decision about kind of mapping is made in patch
> 3 based on PMD request.
> 
> ENA use only one BAR in wc mode and two other without caching,
> therefore not making remap in igb_uio rather not spoil anything.
> 
> I added patch 1 because this variable is provided also outside the
> DPDK to Linux generic functions. I cannot test it with all drivers,
> therefore I make it configurable.
> 
> But general combination of wc and not wc PMD should work, because of patch 3.
> Also if WC is enabled by PMD (like in patch 4) not all BARs will by
> mapped, but only those which has prefetchable flag enabled.
> 
Sounds good, so in that case I've no objection to the patch.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>