Re: [PATCH v2] Pre-emption control for userspace

[Khalid Aziz]

On 03/25/2014 11:44 AM, Andrew Morton wrote:
> So the procfs file is written in binary format and is read back in
> ascii format.  Seems odd.
>
> Perhaps this should all be done as a new syscall rather than some
> procfs thing.

I didn't want to add yet another syscall which will then need to be 
added to glibc, but I am open to doing it through a syscall if that is 
the consensus.

> > +	struct preempt_delay {
> > +		u32 __user *delay_req;		/* delay request flag pointer */
> > +		unsigned char delay_granted:1;	/* currently in delay */
> > +		unsigned char yield_penalty:1;	/* failure to yield penalty */
> > +	} sched_preempt_delay;
>
> The problem with bitfields is that a write to one bitfield can corrupt
> a concurrent write to the other one.  So it's your responsibility to
> provide locking and/or to describe how this race is avoided.  A comment
> here in the definition would be a suitable way of addressing this.

I do not have a strong reason to use a bitfield, just trying to not use 
any more bytes than I need to. If using a char is safer, I would rather 
use safer code.

> > +	if (delay_req) {
> > +		int ret;
> > +
> > +		pagefault_disable();
> > +		ret = __copy_from_user_inatomic(&delay_req_flag, delay_req,
> > +				sizeof(u32));
> > +		pagefault_enable();
>
> This all looks rather hacky and unneccesary.  Can't we somehow use
> plain old get_user() and avoid such fuss?

get_user() takes longer and can sleep if page fault occurs. I need this 
code to be very fast for it to be beneficial and am willing to ignore 
page faults since page fault would imply the task has not touched 
pre-emption delay request field and hence we can resched safely.

> > +#else
> > +#define delay_resched_task(curr) resched_task(curr)
>
> This could have been implemented in C...

Sure, I can do that.

Thanks, Andrew!

--
Khalid

--
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/