On Fri, 2013-01-18 at 18:29 +0000, Christoph Lameter wrote:

On Fri, 18 Jan 2013, Steven Rostedt wrote:

quoted

On Fri, 2013-01-18 at 10:04 -0500, Steven Rostedt wrote:

Just to be more complete:

quoted

	CPU0			CPU1
	----			----

			c = __this_cpu_ptr(s->cpu_slab);
			<migrates to CPU0>

quoted

<cpu fetches c->page>

			<another task>

quoted

			updates c->tid

We can avoid the above scenario by doing a cpu local fetch.

i.e.
`	tid = this_cpu_read(s->cpu_slab->tid);

I'm curious to why not just add the preempt disable? It's rather quick
and avoids all this complex trickery, which is just prone to bugs. It
would make it much easier for others to review as well, and also keeps
the setting of page, objects and cpu_slab consistent with everything
else (which is assigned under preempt(irq)_disable).

quoted

quoted

			updates c->page
			updates c->freelist
<cpu fetches c->tid>
<cpu fetches c->freelist>

  node_match() succeeds even though
    current c->page wont

 <migrates back to CPU 1>

quoted

 this_cpu_cmpxchg_double() only tests
   the object (freelist) and tid, both which
   will match, but the page that was tested
   isn't the right one.

Yes, it's very unlikely, but we are in the business of dealing with the
very unlikely. That's because in our business, the very unlikely is very
likely. Damn, I need to buy a lotto ticket!

Well, the consequence would be that an object from another node than
desired will be allocated. Not that severe of an issue.

Yes, it's not that severe of an issue, but it is still incorrect code.
Why not just allocate on whatever node you want then? Why bother with
the check at all?

-- Steve


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>