Re: PATCH: EDAC - clean up atomic stuff

[Eric W. Biederman]

Andrew Morton <akpm@xxxxxxxx> writes:

> ebiederm@xxxxxxxxxxxx (Eric W. Biederman) wrote:
>>
>> Ok.  If I recall correctly atomic kmaps have the rule that they are
>>  per cpu, and you can't be interrupted when the map is taken, by
>>  something else that will use the map.  But they are safe to use from
>>  interrupt context.  As I recall the code needs to ensure that an
>>  interrupt handler doesn't use the same buffer and that it isn't
>>  preempted where another thread will use the buffer.  The preemption
>>  angle is new since that piece of code was written.
>
> Yes, a particular atomic kmap slot is simply a static, per-cpu scalar. 
> It's just like
>
> int foo[NR_CPUS];
>
> 	...
> 	foo(smp_processor_id());
>
> and all the same rules apply.
>
> The use of KM_BOUNCE_READ does appear to be incorrect.  bounce_copy_vec()
> will use KM_BOUNCE_READ from interrupt context, so if the EDAC code is
> interrupted by the block layer while it holds that kmap, it will find that
> it's suddenly diddling with a different physical page.
>
> So to use KM_BOUNCE_READ, the EDAC code nees to disable local interrupts,
> or to use a different (or new) slot.
>
> In what contexts is edac_mc_scrub_block() called?  If process context, then
> KM_USER0 would suit.

That function is very nice functionality but we could not implement it
properly outside of the kernel.  So the code has been disabled until
just recently. 

Hmm.  Looking at the patch it is most definitely being called from
process context.  Although I think the original was ok from interrupt
context as well.

> Ah, edac_mc_scrub_block() is passing the pageframe address to
> kunmap_atomic() - that's a common bug.  It needs to pass in the virtual
> address which kmap_atomic() returned.

Oops.  Although I am actually surprised kunmap_atomic even needs the address.
Although I can see the kmap type being equally redundant.

There is a much more serious bug there as well.  The code as it
exists is flatly impossible on x86_64 and some other architectures
as they do not support kmap.  It is also broken on x86 as grain can
easily be larger than page size, on old memory controllers where this
is most needed it is the frequently the size of a memory chip select
(aka the size of a single sided DIMM).

We need to do two things.
- Remove a factor from edac_mc_scrub_block (call it edac_mc_scrub_page)
  that simply scrubs a page or maybe a sub page.
- Place the edac_mc_scrub_page which does the kmap and a loop through
  the page contents in arch specific code.

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/