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

Groups > linux.kernel > #1159951 > unrolled thread

Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into the iterant API

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 11/18] jffs2: Convert jffs2_gcd_mtd kthread into  the iterant API Oleg Nesterov <oleg@redhat.com> - 2015-06-06 23:20 +0200
    Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into the  iterant API Jiri Kosina <jkosina@suse.cz> - 2015-06-06 23:40 +0200
      Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into  the iterant API Oleg Nesterov <oleg@redhat.com> - 2015-06-07 00:40 +0200
        Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into the  iterant API Jiri Kosina <jkosina@suse.cz> - 2015-06-07 00:50 +0200
          Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into  the iterant API Oleg Nesterov <oleg@redhat.com> - 2015-06-07 01:10 +0200

#1159951 — Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into the iterant API

On 06/05, Petr Mladek wrote:
>
> [*] In fact, there was a bug in the original code. It tried to process
>     a non-existing signal when the system was freezing. See the common
>     check for pending signal and freezing.

And another bug afaics:

> -			case SIGSTOP:
> -				jffs2_dbg(1, "%s(): SIGSTOP received\n",
> -					  __func__);
> -				set_current_state(TASK_STOPPED);
> -				schedule();
> -				break;

This is obviously racy, we can miss SIGCONT.

Still I personally dislike the new kthread_sigaction() API. I agree,
a couple if signal helpers for kthreads make sense. Say,

	void kthread_do_signal_stop(void)
	{
		spin_lock_irq(&curtent->sighand->siglock);
		if (current->jobctl & JOBCTL_STOP_DEQUEUED)
			__set_current_state(TASK_STOPPED);
		spin_unlock_irq(&current->sighand->siglock);

		schedule();
	}

and probably even "int kthread_signal_deque(void)".

But personally I do not think kthread_do_signal() makes a lot of sense...

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1159959 — Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into the iterant API

On Sat, 6 Jun 2015, Oleg Nesterov wrote:

> Still I personally dislike the new kthread_sigaction() API. I agree,
> a couple if signal helpers for kthreads make sense. Say,
> 
> 	void kthread_do_signal_stop(void)
> 	{
> 		spin_lock_irq(&curtent->sighand->siglock);
> 		if (current->jobctl & JOBCTL_STOP_DEQUEUED)
> 			__set_current_state(TASK_STOPPED);
> 		spin_unlock_irq(&current->sighand->siglock);
> 
> 		schedule();
> 	}

... not to mention the fact that 'STOP' keyword in relation to kthreads 
has completely different meaning today, which just contributes to overall 
confusion; but that's an independent story.

> 
> and probably even "int kthread_signal_deque(void)".
> 
> But personally I do not think kthread_do_signal() makes a lot of sense...

Would it be possible for you to elaborate a little bit more why you think 
so ... ?

I personally don't see a huge principal difference between 
"kthread_signal_dequeue() + kthread_do_signal_{stop,...}" vs. generic 
"kthread_do_signal()" that's just basically completely general and takes 
care of 'everything necessary'. That being said, my relationship to signal 
handling code is of course much less intimate compared to yours, so I am 
really curious what particular objections to that interface have.

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1159967

On 06/06, Jiri Kosina wrote:
>
> On Sat, 6 Jun 2015, Oleg Nesterov wrote:
>
> > Still I personally dislike the new kthread_sigaction() API. I agree,
> > a couple if signal helpers for kthreads make sense. Say,
> >
> > 	void kthread_do_signal_stop(void)
> > 	{
> > 		spin_lock_irq(&curtent->sighand->siglock);
> > 		if (current->jobctl & JOBCTL_STOP_DEQUEUED)
> > 			__set_current_state(TASK_STOPPED);
> > 		spin_unlock_irq(&current->sighand->siglock);
> >
> > 		schedule();
> > 	}
>
> ... not to mention the fact that 'STOP' keyword in relation to kthreads
> has completely different meaning today, which just contributes to overall
> confusion; but that's an independent story.

Yes, agreed.

> > But personally I do not think kthread_do_signal() makes a lot of sense...
>
> Would it be possible for you to elaborate a little bit more why you think
> so ... ?

Please see another email I sent in reply to 06/18.

> I personally don't see a huge principal difference between
> "kthread_signal_dequeue() + kthread_do_signal_{stop,...}" vs. generic
> "kthread_do_signal()" that's just basically completely general and takes
> care of 'everything necessary'.

Then why do we need the new API ?

And I do see the difference. Rightly or not I belive that this API buys
nothing but makes the kthread && signal interaction more complex and
confusing. For no reason.

But!

> That being said, my relationship to signal
> handling code is of course much less intimate compared to yours,

No, no, no, this doesn't matter at all ;)

Yes I do dislike this API. So what? I can be wrong. So if other reviewers
like it I will hate them all ^W^W^W not argure. So please comment. I never
trust myself unless I can technically (try to) prove I am right. In this
case I can't, this is only my feeling.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1159969 — Re: [RFC PATCH 11/18] jffs2: Convert jffs2_gcd_mtd kthread into the iterant API

On Sun, 7 Jun 2015, Oleg Nesterov wrote:

> > > Still I personally dislike the new kthread_sigaction() API. I agree,
> > > a couple if signal helpers for kthreads make sense. Say,
> > >
> > > 	void kthread_do_signal_stop(void)
> > > 	{
> > > 		spin_lock_irq(&curtent->sighand->siglock);
> > > 		if (current->jobctl & JOBCTL_STOP_DEQUEUED)
> > > 			__set_current_state(TASK_STOPPED);
> > > 		spin_unlock_irq(&current->sighand->siglock);
> > >
> > > 		schedule();
> > > 	}
> >
> > ... not to mention the fact that 'STOP' keyword in relation to kthreads
> > has completely different meaning today, which just contributes to overall
> > confusion; but that's an independent story.
> 
> Yes, agreed.

I BTW think this really needs to be fixed. We have kthread parking and 
kthread stopping at least (and that doesn't include kthreads that actually 
*do* handle SIGSTOP), and the naming is unfortunate and confusing.

> > > But personally I do not think kthread_do_signal() makes a lot of sense...
> >
> > Would it be possible for you to elaborate a little bit more why you think
> > so ... ?
> 
> Please see another email I sent in reply to 06/18.

Yeah, let's just continue more detailed discussion there. I saw your reply 
only after I answered to your original mail.

> > I personally don't see a huge principal difference between 
> > "kthread_signal_dequeue() + kthread_do_signal_{stop,...}" vs. generic 
> > "kthread_do_signal()" that's just basically completely general and 
> > takes care of 'everything necessary'.
> 
> Then why do we need the new API ?

Well, in a nutshell, because of the "it's general and takes care of 
everything" part.

> And I do see the difference. Rightly or not I belive that this API buys
> nothing but makes the kthread && signal interaction more complex and
> confusing. For no reason.

Current situation with kthrads is a mess. Everyone and his grand-son needs 
to put explicit try_to_freeze(), cond_resched() and whatever else checks 
in his private kthreads main loop.

The fact that kthreads are more and more prone to making use of signal 
handling makes this even more complicated, because everyone is reinventing 
his own wheel for this.

It can't be really properly reviewed and - more importantly - it makes the 
whole "how would this particular kthread react to this particular signal?" 
very unpredictable and hard to answer question.

IMO Petr's patchset basically brings some kind order to this all -- it 
"batches" the kthread executions to individual iterations of the main 
loop, and allows to perform actions on the border of the iteration (such 
as, but not limited to, handling of the signals). Signal handling is just 
one of the piggy-backers on top of this general cleanup.

Thanks,

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1159971

On 06/07, Jiri Kosina wrote:
>
> On Sun, 7 Jun 2015, Oleg Nesterov wrote:
>
> > > I personally don't see a huge principal difference between
> > > "kthread_signal_dequeue() + kthread_do_signal_{stop,...}" vs. generic
> > > "kthread_do_signal()" that's just basically completely general and
> > > takes care of 'everything necessary'.
> >
> > Then why do we need the new API ?
>
> Well, in a nutshell, because of the "it's general and takes care of
> everything" part.

...

> Signal handling is just
> one of the piggy-backers on top of this general cleanup.

And to avoid the confusion: so far I only argued with the signal
handling part of this API. Namely with kthread_do_signal(), especially
with the SIG_DFL logic.

If we want somthing like kthread_iterant agree it should probably help to
handle the signals too. But afaics kthread_do_signal() doesn't really help
and certainly it is not strictly necessary.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web