Re: [RFC Patch] use MTRR for write combining if PAT is not available

[Thomas Schlichter <thomas.schlichter@web.de>]

Suresh Siddha wrote:
> On Wed, 2009-10-21 at 13:01 -0700, Thomas Schlichter wrote:
> > OK, so I think the attached patches should do it. Hopefully I have addressed 
> > all your comments.
> 
> Thomas,
> 
> I have couple of issues with this patchset still. pci_mmap_page_range()
> doesn't get called for each fork(). So, we won't be ref counting the
> mtrr usage properly.

When forking, what happens with the "struct file"? If it is being copied, then the
processes share the same private data which would be freed during the first
release(). I think this would be a problem whereever file-private data are used.

So I think it must be shared between the forked processes and some reference
counting must exist. This reference counting must ensure that release() is only
called when all processes did close() their file.

And in that case (shared "struct file", one single release() call in the end) this
implementation should be completely safe...

> I need to think a bit more carefully on this. Can I get back to you
> early next week on this, as I am traveling and need to think through
> this?

Of course you can. But as I do this just in my spare time, I can only work on this
in the evenings...

> We already keep track of some of the PAT ref counting using
> track_pfn_vma_copy(). And we need to extend/use something similar here.
> 
> Even if we need to extend sysfs or pci vma ops, we need to increment and
> decrement the ref count of the mtrr register that gets used.

The MTRR register ref count is already incremented with each mtrr_add() or
mtrr_add_page() that has the "increment" parameter set to true. Which is the
case in my implementation.

> There is no need to go through num_var_ranges etc.

Well I have to remember wich file added which MTRR entries. Because I have
to remove them if the file is being closed. Therefore I need an array of size
"num_var_ranges" (or MTRR_MAX_VAR_RANGES which is the uper bound).

Btw. if there really is a problem in my reference counting code, it also exists in
the current code in:
  arch/x86/kernel/cpu/mtrr/if.c

My patch borrows its ref-counting parts from there...

Kind regards,
  Thomas