Re: [RFC PATCH] percpu system call: fast userspace percpu critical sections

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- Original Message -----
> On Thu, May 21, 2015 at 10:44:47AM -0400, Mathieu Desnoyers wrote:
> 
> > +struct thread_percpu_user {
> > +	int32_t nesting;
> > +	int32_t signal_sent;
> > +	int32_t signo;
> > +	int32_t current_cpu;
> > +};
> 
> I would require this thing be naturally aligned, such that it does not
> cross cacheline boundaries.

Good point. Adding a comment into the code to that effect.

> 
> > +
> > +static void percpu_user_sched_in(struct preempt_notifier *notifier, int
> > cpu)
> > +{
> > +	struct thread_percpu_user __user *tpu_user;
> > +	struct thread_percpu_user tpu;
> > +	struct task_struct *t = current;
> > +
> > +	tpu_user = t->percpu_user;
> > +	if (tpu_user == NULL)
> > +		return;
> > +	if (unlikely(t->flags & PF_EXITING))
> > +		return;
> > +	/*
> > +	 * access_ok() of tpu_user has already been checked by sys_percpu().
> > +	 */
> > +	if (__put_user(smp_processor_id(), &tpu_user->current_cpu)) {
> > +		WARN_ON_ONCE(1);
> > +		return;
> > +	}
> 
> This seems a waste; you already read the number unconditionally, might
> as well double check and avoid the store.
> 
> > +	if (__copy_from_user(&tpu, tpu_user, sizeof(tpu))) {
> > +		WARN_ON_ONCE(1);
> > +		return;
> > +	}
> 
> 	if (tpu.current_cpu != smp_processor_id())
> 		__put_user();

Yep, and I could even use the "cpu" parameter received by the
function rather than smp_processor_id().

> 
> 
> 
> > +	if (!tpu.nesting || tpu.signal_sent)
> > +		return;
> > +	if (do_send_sig_info(tpu.signo, SEND_SIG_PRIV, t, 0)) {
> > +		WARN_ON_ONCE(1);
> > +		return;
> > +	}
> > +	tpu.signal_sent = 1;
> > +	if (__copy_to_user(tpu_user, &tpu, sizeof(tpu))) {
> > +		WARN_ON_ONCE(1);
> > +		return;
> > +	}
> > +}
> 
> Please do not use preempt notifiers for this.

Do you recommend we issue a function call from the scheduler
finish_task_switch() ?

> 
> Second, this all is done with preemption disabled, this means that all
> that user access can fail.

OK, this is one part I was worried about.

> 
> You print useless WARNs and misbehave. If you detect a desire to fault,
> you could delay until return to userspace and try again there. But it
> all adds complexity.

We could keep a flag, and then call the function again if we detect a
desire to fault.

> 
> The big advantage pjt's scheme had is that we have the instruction
> pointer, we do not need to go read userspace memory that might not be
> there. And it being limited  to a single range, while inconvenient,
> simplifies the entire kernel side to:
> 
> 	if ((unsigned long)(ip - offset) < size)
> 		do_magic();
> 
> Which is still simpler than the above.

There is one big aspect of pjt's approach that I still don't grasp
after all this time that makes me worry. How does it interact with
the following scenario ?

Userspace thread
  - within the code region that needs to be restarted
    - signal handler nested on top
      - running within the signal handler code
        - preempted by kernel
          - checking instruction pointer misses the userspace stack
            underneath the signal handler.

Given this scenario, is the kernel code really as simple as a pointer check
on pt_regs, or do we need a stack walk over all signal frames ? Another way
would be to check for the pt_regs instruction pointer whenever we receive
a signal, but then it would require per-architectures modifications, and
suddenly becomes less straightforward.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com