Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

Hey Andy, thanks for your review,  adding Hyong-Youb Kim for  review of the
full range ioremap_wc() idea below.

On Wed, Apr 15, 2015 at 06:38:51PM -0400, Andy Walls wrote:
> Hi All,
> 
> On Mon, 2015-04-13 at 19:49 +0200, Luis R. Rodriguez wrote:
> > From the beginning it seems only framebuffer devices used MTRR/WC,
> [snip]
> >  The ivtv device is a good example of the worst type of
> > situations and these days. So perhap __arch_phys_wc_add() and a
> > ioremap_ucminus() might be something more than transient unless hardware folks
> > get a good memo or already know how to just Do The Right Thing (TM).
> 
> Just to reiterate a subtle point, use of the ivtvfb is *optional*.  A
> user may or may not load it.  When the user does load the ivtvfb driver,
> the ivtv driver has already been initialized and may have functions of
> the card already in use by userspace.

I suspected this and its why I note that a rewrite to address a clean
split with separate ioremap seems rather difficult in this case.

> Hopefully no one is trying to use the OSD as framebuffer and the video
> decoder/output engine for video display at the same time. 

Worst case concern I have also is the implications of having overlapping
ioremap() calls (as proposed in my last reply) for different memory types
and having the different virtual memory addresse used by different parts
of the driver. Its not clear to me what the hardware implications of this
are.

>  But the video
> decoder/output device nodes may already be open for performing ioctl()
> functions so unmapping the decoder IO space out from under them, when
> loading the ivtvfb driver module, might not be a good thing. 

Using overlapping ioremap() calls with different memory types would address
this concern provided hardware won't barf both on the device and CPU. Hardware
folks could provide feedback or an ivtvfb user could test the patch supplied
on both non-PAT and PAT systems. Even so, who knows,  this might work on some
systems while not on others, only hardware folks would know.

An alternative... is to just ioremap_wc() the entire region, including
MMIO registers for these old devices. I see one ethernet driver that does
this, myri10ge, and am curious how and why they ended up deciding this
and if they have run into any issues. I wonder if this is a reasonable
comrpomise for these 2 remaining corner cases.

  Luis

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Andy Walls]

On Thu, 2015-04-16 at 01:58 +0200, Luis R. Rodriguez wrote:
> Hey Andy, thanks for your review,  adding Hyong-Youb Kim for  review of the
> full range ioremap_wc() idea below.
> 
> On Wed, Apr 15, 2015 at 06:38:51PM -0400, Andy Walls wrote:
> > Hi All,
> > 
> > On Mon, 2015-04-13 at 19:49 +0200, Luis R. Rodriguez wrote:
> > > From the beginning it seems only framebuffer devices used MTRR/WC,
> > [snip]
> > >  The ivtv device is a good example of the worst type of
> > > situations and these days. So perhap __arch_phys_wc_add() and a
> > > ioremap_ucminus() might be something more than transient unless hardware folks
> > > get a good memo or already know how to just Do The Right Thing (TM).
> > 
> > Just to reiterate a subtle point, use of the ivtvfb is *optional*.  A
> > user may or may not load it.  When the user does load the ivtvfb driver,
> > the ivtv driver has already been initialized and may have functions of
> > the card already in use by userspace.
> 
> I suspected this and its why I note that a rewrite to address a clean
> split with separate ioremap seems rather difficult in this case.
> 
> > Hopefully no one is trying to use the OSD as framebuffer and the video
> > decoder/output engine for video display at the same time. 
> 
> Worst case concern I have also is the implications of having overlapping
> ioremap() calls (as proposed in my last reply) for different memory types
> and having the different virtual memory addresse used by different parts
> of the driver. Its not clear to me what the hardware implications of this
> are.
> 
> >  But the video
> > decoder/output device nodes may already be open for performing ioctl()
> > functions so unmapping the decoder IO space out from under them, when
> > loading the ivtvfb driver module, might not be a good thing. 
> 
> Using overlapping ioremap() calls with different memory types would address
> this concern provided hardware won't barf both on the device and CPU. Hardware
> folks could provide feedback or an ivtvfb user could test the patch supplied
> on both non-PAT and PAT systems. Even so, who knows,  this might work on some
> systems while not on others, only hardware folks would know.

The CX2341[56] firmware+hardware has a track record for being really
picky about sytem hardware.  It's primary symptoms are for the DMA
engine or Mailbox protocol to get hung up.  So yeah, it could barf
easily on some users.

> An alternative... is to just ioremap_wc() the entire region, including
> MMIO registers for these old devices.

That's my thought; as long as implementing PCI write then read can force
writes to be posted and that setting that many pages as WC doesn't cause
some sort of PAT resource exhaustion. (I know very little about PAT).

>  I see one ethernet driver that does
> this, myri10ge, and am curious how and why they ended up deciding this
> and if they have run into any issues. I wonder if this is a reasonable
> comrpomise for these 2 remaining corner cases.
> 
>   Luis

Regards,
Andy

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Hyong-Youb Kim]

On Thu, Apr 16, 2015 at 01:58:16AM +0200, Luis R. Rodriguez wrote:
> 
> An alternative... is to just ioremap_wc() the entire region, including
> MMIO registers for these old devices. I see one ethernet driver that does
> this, myri10ge, and am curious how and why they ended up deciding this
> and if they have run into any issues. I wonder if this is a reasonable
> comrpomise for these 2 remaining corner cases.
> 

For myri10ge, it a performance thing.  Descriptor rings are in NIC
memory BAR0, not in host memory.  Say, to send a packet, the driver
writes the send descriptor to the ioremap'd NIC memory.  It is a
multi-word descriptor.  So, to send it as one PCIE MWr transaction,
the driver maps the whole BAR0 as WC and does "copy descriptor; wmb".

Without WC, descriptors would end up as multiple 4B or 8B MWr packets
to the NIC, which has a pretty big performance impact on this
particular NIC.

Most registers that do not want WC are actually in BAR2, which is not
mapped as WC.  For registers that are in BAR0, we do "write to the
register; wmb".  If we want to wait till the NIC has seen the write,
we do "write; wmb; read".

This approach has worked for this device for many years.  I cannot say
whether it works for other devices, though.

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Thu, Apr 16, 2015 at 01:18:37PM +0900, Hyong-Youb Kim wrote:
> On Thu, Apr 16, 2015 at 01:58:16AM +0200, Luis R. Rodriguez wrote:
> > 
> > An alternative... is to just ioremap_wc() the entire region, including
> > MMIO registers for these old devices. I see one ethernet driver that does
> > this, myri10ge, and am curious how and why they ended up deciding this
> > and if they have run into any issues. I wonder if this is a reasonable
> > comrpomise for these 2 remaining corner cases.
> > 
> 
> For myri10ge, it a performance thing.  Descriptor rings are in NIC
> memory BAR0, not in host memory.  Say, to send a packet, the driver
> writes the send descriptor to the ioremap'd NIC memory.  It is a
> multi-word descriptor.  So, to send it as one PCIE MWr transaction,
> the driver maps the whole BAR0 as WC and does "copy descriptor; wmb".

Interesting, so you burst write multi-word descriptor writes using
write-combining here for the Ethernet device.

> Without WC, descriptors would end up as multiple 4B or 8B MWr packets
> to the NIC, which has a pretty big performance impact on this
> particular NIC.

How big are the descriptors?

> Most registers that do not want WC are actually in BAR2, which is not
> mapped as WC.  For registers that are in BAR0, we do "write to the
> register; wmb".  If we want to wait till the NIC has seen the write,
> we do "write; wmb; read".

Interesting, thanks, yeah using this as a work around to the problem sounds
plausible however it still would require likely making just as many changes to
the ivtv and ipath driver as to just do a proper split. I do wonder however if
this sort of work around can be generalized somehow though so that others could
use, if this sort of thing is going to become prevalent. If so then this would
serve two purposes: work around for the corner cases of MTRR use on Linux and
also these sorts of device constraints.

In order to determine if this is likely to be generally useful could you elaborate
a bit more about the detals of the performance issues of not bursting writes
for the descriptor on this device.

Even if that is done a conversion over to this work around seems it may require
device specific nitpicks. For instance I note in myri10ge_submit_req() for
small writes you just do a reverse write and do the first set last, then
finally the last 32 bits are rewritten, I guess to trigger something?

> This approach has worked for this device for many years.  I cannot say
> whether it works for other devices, though.

I think it should but the more interesting question would be exactly
*why* it was needed for this device, who determined that, and why?

  Luis

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Hyong-Youb Kim]

On Thu, Apr 16, 2015 at 08:54:26PM +0200, Luis R. Rodriguez wrote:
> > Without WC, descriptors would end up as multiple 4B or 8B MWr packets
> > to the NIC, which has a pretty big performance impact on this
> > particular NIC.
> 
> How big are the descriptors?

Some are 64B (a batch of eight 8B descriptors).  Some are 16B.

> > Most registers that do not want WC are actually in BAR2, which is not
> > mapped as WC.  For registers that are in BAR0, we do "write to the
> > register; wmb".  If we want to wait till the NIC has seen the write,
> > we do "write; wmb; read".
> 
> Interesting, thanks, yeah using this as a work around to the problem sounds
> plausible however it still would require likely making just as many changes to
> the ivtv and ipath driver as to just do a proper split. I do wonder however if
> this sort of work around can be generalized somehow though so that others could
> use, if this sort of thing is going to become prevalent. If so then this would
> serve two purposes: work around for the corner cases of MTRR use on Linux and
> also these sorts of device constraints.

These Myricom devices are very non-standard in my opinion, at least in
the Ethernet world.  Few, if any, other devices depend so much on WC
like these do.  I think almost all devices now have rings in host
memory.  The NIC pulls them via DMA.  No need for WC, and no need to
special case registers...

> In order to determine if this is likely to be generally useful could you elaborate
> a bit more about the detals of the performance issues of not bursting writes
> for the descriptor on this device.

For this particular Myricom device, performance penalty stems from the
use of slow path in the firmware.  They are not about how effectively
we use PCI Express or latency or bandwidth.  Small MWr packets end up
casuing slow path processing via the firmware in this device.

There are HPC low latency NICs that use WC for different reasons.  To
reduce latency as much as possible, some of these copy small packets
to the NIC memory via PIO (BAR0, and so on), instead of DMA.  They
want WC mapping to minimize PCI Express packets/transactions.

I do not know about video adapters and their use for WC.

> Even if that is done a conversion over to this work around seems it may require
> device specific nitpicks. For instance I note in myri10ge_submit_req() for
> small writes you just do a reverse write and do the first set last, then
> finally the last 32 bits are rewritten, I guess to trigger something?

Right.  The device polls the last word to start sending, DMA, etc.

> > This approach has worked for this device for many years.  I cannot say
> > whether it works for other devices, though.
> 
> I think it should but the more interesting question would be exactly
> *why* it was needed for this device, who determined that, and why?

Hopefully, the above text answers some of your questions.

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Wed, Apr 15, 2015 at 09:07:37PM -0400, Andy Walls wrote:
> On Thu, 2015-04-16 at 01:58 +0200, Luis R. Rodriguez wrote:
> > Hey Andy, thanks for your review,  adding Hyong-Youb Kim for  review of the
> > full range ioremap_wc() idea below.
> > 
> > On Wed, Apr 15, 2015 at 06:38:51PM -0400, Andy Walls wrote:
> > > Hi All,
> > > 
> > > On Mon, 2015-04-13 at 19:49 +0200, Luis R. Rodriguez wrote:
> > > > From the beginning it seems only framebuffer devices used MTRR/WC,
> > > [snip]
> > > >  The ivtv device is a good example of the worst type of
> > > > situations and these days. So perhap __arch_phys_wc_add() and a
> > > > ioremap_ucminus() might be something more than transient unless hardware folks
> > > > get a good memo or already know how to just Do The Right Thing (TM).
> > > 
> > > Just to reiterate a subtle point, use of the ivtvfb is *optional*.  A
> > > user may or may not load it.  When the user does load the ivtvfb driver,
> > > the ivtv driver has already been initialized and may have functions of
> > > the card already in use by userspace.
> > 
> > I suspected this and its why I note that a rewrite to address a clean
> > split with separate ioremap seems rather difficult in this case.
> > 
> > > Hopefully no one is trying to use the OSD as framebuffer and the video
> > > decoder/output engine for video display at the same time. 
> > 
> > Worst case concern I have also is the implications of having overlapping
> > ioremap() calls (as proposed in my last reply) for different memory types
> > and having the different virtual memory addresse used by different parts
> > of the driver. Its not clear to me what the hardware implications of this
> > are.
> > 
> > >  But the video
> > > decoder/output device nodes may already be open for performing ioctl()
> > > functions so unmapping the decoder IO space out from under them, when
> > > loading the ivtvfb driver module, might not be a good thing. 
> > 
> > Using overlapping ioremap() calls with different memory types would address
> > this concern provided hardware won't barf both on the device and CPU. Hardware
> > folks could provide feedback or an ivtvfb user could test the patch supplied
> > on both non-PAT and PAT systems. Even so, who knows,  this might work on some
> > systems while not on others, only hardware folks would know.
> 
> The CX2341[56] firmware+hardware has a track record for being really
> picky about sytem hardware.  It's primary symptoms are for the DMA
> engine or Mailbox protocol to get hung up.  So yeah, it could barf
> easily on some users.
> 
> > An alternative... is to just ioremap_wc() the entire region, including
> > MMIO registers for these old devices.
> 
> That's my thought; as long as implementing PCI write then read can force
> writes to be posted and that setting that many pages as WC doesn't cause
> some sort of PAT resource exhaustion. (I know very little about PAT).

So upon review that strategy won't work well unless we implemnt some
sort of of hack on the driver. That's also quite a bit of work.

Andy, can we live without MTRR support on this driver for future kernels? This
would only leave ipath as the only offending driver.

  Luis

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Tue, Apr 21, 2015 at 3:02 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> Andy, can we live without MTRR support on this driver for future kernels? This
> would only leave ipath as the only offending driver.

Sorry to be clear, can we live with removal of write-combining on this driver?

 Luis

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Andy Lutomirski]

On Tue, Apr 21, 2015 at 3:08 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Apr 21, 2015 at 3:02 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> Andy, can we live without MTRR support on this driver for future kernels? This
>> would only leave ipath as the only offending driver.
>
> Sorry to be clear, can we live with removal of write-combining on this driver?
>

I personally think so, but a driver maintainer's ack would be nice.

--Andy

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Tue, Apr 21, 2015 at 06:51:26PM -0400, Andy Walls wrote:
> Sorry for the top post; mobile work email account.
> 
> Luis,
> 
> You do the changes to remove MTTR and point me to your dev repo and branch.
> Also point me to the new functions/primitives I'll need.

There is nothing new actually needed for ivtv, unless of course
the ivtv driver is bounded to use a large MTRR that includes
the non-framebuffer region, if so then the ioremap_uc() would
be needed, and you can just cherry pick that patch:

https://marc.info/?l=linux-kernel&m=142964809110516&w=1

I'll bounce that patch to you as well. Might help reading this
patch too:

https://marc.info/?l=linux-kernel&m=142964809710517&w=1

If your write-combining area is not restricted by size constraints
so that it also include the non-framebuffer areas then you can just
do a simple conversion of the driver to use ioremap_wc() on the
framebuffer followed by arch_phys_wc_add().

An example driver that required changes to split this with size
contraints is atyfb, here are the changes for it:

https://marc.info/?l=linux-kernel&m=142964818810539&w=1
https://marc.info/?l=linux-kernel&m=142964813610531&w=1
https://marc.info/?l=linux-kernel&m=142964811010524&w=1
https://marc.info/?l=linux-kernel&m=142964814810532&w=1

If you are not constrained by MTRR's limitation on size then
a simple trivial driver conversion is sufficient. For example:

https://marc.info/?l=linux-kernel&m=142964744610286&w=1

I should also note that we are strivoing to also not use overlapping ioremap()
calls as we want to avoid that mess. Overlapping iroemap() calls with different
types could in theory work but its best we just design clean drivers and avoid
this.

As per Andy Lutomirski, what we'd need done on ivtv likely is
for it to ioremap() for an initial bring up of the device, then
infer the framebuffer offset, and only when that is being used
then iounmap and then ioremap() again split areas on the driver,
one with ioremap.

> I'll do the changes to add write-combining back into ivtv and ivtvfb, test
> them with my hardware and push them to my linuxtv.org git repo.

Great! The above sounded like a complexity you did not wish to
take on, but if you're up for the change, that'd be awesome!

> I know there is at least one English speaking user in India using ivtv with
> old PVR hardware, and probably folks in less developed places also using it.

If the above is too much work for that few amount of users I'd hope
we can just have them use older kernels, for the sake of sane APIs and
clean driver architecture.

  Luis