sparc64: Don't pass a pointer to xcall_flush_tlb_pending

The use of the vaddrs array to pass the page addresses to the remote
functions is lacking serialization. xmp_cross_call_masked() returns when
the mondo signals are delivered to the target cpus, not when the remote
functions have completed. This can result in the vaddr array being
overwritten by the initiating cpu before the remote functions execute.

Failing to flush the tlb entries correctly can cause user-space
processes to access stale memory resulting in various traps. An easy
way to trigger this behavior is to repeatedly write a large number to
/proc/sys/vm/nr_hugepages:

while true ; do
	echo 1000000 > /proc/sys/vm/nr_hugepages
done

The very limited environment in which the remote functions execute makes
it difficult to pass any more arguments. This patch passes one or two
page addresses as arguments eliminating the pointer argument. In order
to limit the number of cross calls, if the number of tlb entries to flush
exceeds a threshold, xcall_tlb_flush_mm is called instead. I've initially
set the threshold at 16, but there is probably a better number.

Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Kleikamp <dave.kleikamp@oracle.com>
Date: Wed, 17 Apr 2013 15:52:26 -0500

> +	else if (nr < 17) {
> +		int i;
> +		unsigned long vaddr2;
> +		for(i = 0; i < nr; i+=2) {

Order the local variables from longest line to shortest.

Put an empty line between the last local variable and the
first statement.

"for" is C primitive, therefor you put a space between it
and the openning parenthesis.

> +			vaddr2 = i + 1 < nr ? vaddrs[i + 1] : 0;
> +			smp_cross_call_masked(&xcall_flush_tlb_pending,
> +					      ctx, vaddrs[i], vaddr2,
> +					      mm_cpumask(mm));
> +		}
> +	} else
> +		/*
> +		 * At some point it is probably cheaper to flush the whole
> +		 * context rather than make too many cross calls
> +		 */
> +		smp_cross_call_masked(&xcall_flush_tlb_mm, ctx, 0, 0,
>  				      mm_cpumask(mm));
>  
> +

Format comments:

	/* Like
	 * this.
	 */

Multi-line basic blocks need to be enclosed in braces, even if it is
more than one line because of comments.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I also don't agree with this approach to fix the bug.

I'd much rather we properly synchronize.  One idea is to have
a pendulum which sways every time a remove batch flush is
performed.

The pendulum is a pointer, which points to one of two per-cpu batch
datastructures.  A remote flush is issued by setting an atomic counter
to the number of remote cpus doing the flush in the per-cpu batch
area, each remote cpu decrements the atomic counter.

The submitter also "sways the pendulum" by adjusting the pointer to
other per-cpu batch datastructure.

If a submitter sees that there is a reference count on the per-cpu
batch the pendulum points to, it waits until the counter hits zero.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 04/17/2013 04:31 PM, David Miller wrote:
> 
> I also don't agree with this approach to fix the bug.

I'm not wild about it myself, but it's relatively simple and fixes the
problem.

> I'd much rather we properly synchronize.  One idea is to have
> a pendulum which sways every time a remove batch flush is
> performed.
> 
> The pendulum is a pointer, which points to one of two per-cpu batch
> datastructures.  A remote flush is issued by setting an atomic counter
> to the number of remote cpus doing the flush in the per-cpu batch
> area, each remote cpu decrements the atomic counter.

The remote function doesn't operate with a stack, so it can't simply
call atomic_dec (or atomic_sub) but would need to open code it.
Possible, but it makes the fix a bit more complicated.

> The submitter also "sways the pendulum" by adjusting the pointer to
> other per-cpu batch datastructure.
> 
> If a submitter sees that there is a reference count on the per-cpu
> batch the pendulum points to, it waits until the counter hits zero.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Dave Kleikamp <dave.kleikamp@oracle.com>
Date: Wed, 17 Apr 2013 17:07:26 -0500

> The remote function doesn't operate with a stack, so it can't simply
> call atomic_dec (or atomic_sub) but would need to open code it.
> Possible, but it makes the fix a bit more complicated.

It's not that difficult, I implemented a patch and will post it
after I'm done debugging it.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html