Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1372508 > unrolled thread

Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a task has been preempted

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a  task has been preempted Petr Mladek <pmladek@suse.com> - 2016-04-06 15:10 +0200
    Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a  task has been preempted Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-06 18:40 +0200
      Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a  task has been preempted Petr Mladek <pmladek@suse.com> - 2016-04-07 11:50 +0200
        Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a  task has been preempted Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-07 16:40 +0200
          Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a  task has been preempted Petr Mladek <pmladek@suse.com> - 2016-04-08 10:10 +0200
            Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a  task has been preempted Josh Poimboeuf <jpoimboe@redhat.com> - 2016-04-08 16:40 +0200

#1372508 — Re: [RFC PATCH v1.9 05/14] sched: horrible way to detect whether a task has been preempted

On Fri 2016-03-25 14:34:52, Josh Poimboeuf wrote:
> This is a horrible way to detect whether a task has been preempted.
> Come up with something better: task flag?  or is there already an
> existing mechanism?

What about using kallsyms_lookup_size_offset() to check the address.
It is more heavyweight but less hacky. The following code seems
to work for me:

bool in_preempt_schedule_irq(unsigned long addr)
{
	static unsigned long size;

	if (unlikely(!size)) {
		int ret;

		ret = kallsyms_lookup_size_offset(
				(unsigned long)preempt_schedule_irq,
				size, NULL);

		/*
		 * Warn when the function is used without kallsyms or
		 * when it is unable to locate preempt_schedule_irq().
		 * Be conservative and always return true in this case.
		 */
		if (WARN_ON(!ret))
			size = -1L;
	}

	return (addr - (unsigned long)preempt_schedule_irq <= size);
}


Best Regards,
Petr

[toc] | [next] | [standalone]

#1372655

On Wed, Apr 06, 2016 at 03:06:19PM +0200, Petr Mladek wrote:
> On Fri 2016-03-25 14:34:52, Josh Poimboeuf wrote:
> > This is a horrible way to detect whether a task has been preempted.
> > Come up with something better: task flag?  or is there already an
> > existing mechanism?
> 
> What about using kallsyms_lookup_size_offset() to check the address.
> It is more heavyweight but less hacky. The following code seems
> to work for me:
> 
> bool in_preempt_schedule_irq(unsigned long addr)
> {
> 	static unsigned long size;
> 
> 	if (unlikely(!size)) {
> 		int ret;
> 
> 		ret = kallsyms_lookup_size_offset(
> 				(unsigned long)preempt_schedule_irq,
> 				size, NULL);
> 
> 		/*
> 		 * Warn when the function is used without kallsyms or
> 		 * when it is unable to locate preempt_schedule_irq().
> 		 * Be conservative and always return true in this case.
> 		 */
> 		if (WARN_ON(!ret))
> 			size = -1L;
> 	}
> 
> 	return (addr - (unsigned long)preempt_schedule_irq <= size);
> }

Yeah, that would definitely be better.  Though still somewhat hacky.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1373225

On Wed 2016-04-06 11:33:56, Josh Poimboeuf wrote:
> On Wed, Apr 06, 2016 at 03:06:19PM +0200, Petr Mladek wrote:
> > On Fri 2016-03-25 14:34:52, Josh Poimboeuf wrote:
> > > This is a horrible way to detect whether a task has been preempted.
> > > Come up with something better: task flag?  or is there already an
> > > existing mechanism?
> > 
> > What about using kallsyms_lookup_size_offset() to check the address.
> > It is more heavyweight but less hacky. The following code seems
> > to work for me:
> > 
> > bool in_preempt_schedule_irq(unsigned long addr)
> > {
> > 	static unsigned long size;
> > 
> > 	if (unlikely(!size)) {
> > 		int ret;
> > 
> > 		ret = kallsyms_lookup_size_offset(
> > 				(unsigned long)preempt_schedule_irq,
> > 				size, NULL);
				^^^^
It works even better with &size ;-)

> > 
> > 		/*
> > 		 * Warn when the function is used without kallsyms or
> > 		 * when it is unable to locate preempt_schedule_irq().
> > 		 * Be conservative and always return true in this case.
> > 		 */
> > 		if (WARN_ON(!ret))
> > 			size = -1L;
> > 	}
> > 
> > 	return (addr - (unsigned long)preempt_schedule_irq <= size);
> > }
> 
> Yeah, that would definitely be better.  Though still somewhat hacky.

Yeah. Well this is the same approach that we use to check if a patched
function is on the stack. We could even move this check into the
livepatch code but then print_context_stack_reliable() will not
always give reliable results.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1373434

On Thu, Apr 07, 2016 at 11:47:00AM +0200, Petr Mladek wrote:
> On Wed 2016-04-06 11:33:56, Josh Poimboeuf wrote:
> > On Wed, Apr 06, 2016 at 03:06:19PM +0200, Petr Mladek wrote:
> > > On Fri 2016-03-25 14:34:52, Josh Poimboeuf wrote:
> > > > This is a horrible way to detect whether a task has been preempted.
> > > > Come up with something better: task flag?  or is there already an
> > > > existing mechanism?
> > > 
> > > What about using kallsyms_lookup_size_offset() to check the address.
> > > It is more heavyweight but less hacky. The following code seems
> > > to work for me:
> > > 
> > > bool in_preempt_schedule_irq(unsigned long addr)
> > > {
> > > 	static unsigned long size;
> > > 
> > > 	if (unlikely(!size)) {
> > > 		int ret;
> > > 
> > > 		ret = kallsyms_lookup_size_offset(
> > > 				(unsigned long)preempt_schedule_irq,
> > > 				size, NULL);
> 				^^^^
> It works even better with &size ;-)
> 
> > > 
> > > 		/*
> > > 		 * Warn when the function is used without kallsyms or
> > > 		 * when it is unable to locate preempt_schedule_irq().
> > > 		 * Be conservative and always return true in this case.
> > > 		 */
> > > 		if (WARN_ON(!ret))
> > > 			size = -1L;
> > > 	}
> > > 
> > > 	return (addr - (unsigned long)preempt_schedule_irq <= size);
> > > }
> > 
> > Yeah, that would definitely be better.  Though still somewhat hacky.
> 
> Yeah. Well this is the same approach that we use to check if a patched
> function is on the stack.

Oh, I agree that it's a good way to check if preempt_schedule_irq() is
on the stack.  I'm just not convinced that's the cleanest way to ask
"has this task been preempted".

> We could even move this check into the livepatch code but then
> print_context_stack_reliable() will not always give reliable results.

Why would moving the check to the livepatch code affect the reliability
of print_context_stack_reliable()?

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1374062

On Thu 2016-04-07 09:34:03, Josh Poimboeuf wrote:
> On Thu, Apr 07, 2016 at 11:47:00AM +0200, Petr Mladek wrote:
> > On Wed 2016-04-06 11:33:56, Josh Poimboeuf wrote:
> > > On Wed, Apr 06, 2016 at 03:06:19PM +0200, Petr Mladek wrote:
> > We could even move this check into the livepatch code but then
> > print_context_stack_reliable() will not always give reliable results.
> 
> Why would moving the check to the livepatch code affect the reliability
> of print_context_stack_reliable()?

print_context_stack_reliable() is a generic function that might
eventualy be used also outside livepatch code. If there is
preempt_schedule_irq() on the stack, it means that the rest
of the stack might be unreliable and it should be detected
by the function itself.

Let's forget the idea of moving the check into the livepatch
code :-)

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1374254

On Fri, Apr 08, 2016 at 10:07:10AM +0200, Petr Mladek wrote:
> On Thu 2016-04-07 09:34:03, Josh Poimboeuf wrote:
> > On Thu, Apr 07, 2016 at 11:47:00AM +0200, Petr Mladek wrote:
> > > On Wed 2016-04-06 11:33:56, Josh Poimboeuf wrote:
> > > > On Wed, Apr 06, 2016 at 03:06:19PM +0200, Petr Mladek wrote:
> > > We could even move this check into the livepatch code but then
> > > print_context_stack_reliable() will not always give reliable results.
> > 
> > Why would moving the check to the livepatch code affect the reliability
> > of print_context_stack_reliable()?
> 
> print_context_stack_reliable() is a generic function that might
> eventualy be used also outside livepatch code. If there is
> preempt_schedule_irq() on the stack, it means that the rest
> of the stack might be unreliable and it should be detected
> by the function itself.

Ah, I see now.  I actually thought you meant something else (moving
in_preempt_schedule_irq() itself to livepatch code, but still calling it
from print_context_stack_reliable()).

> Let's forget the idea of moving the check into the livepatch
> code :-)

Agreed :-)

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web