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

Groups > linux.kernel > #1241279 > unrolled thread

Re: [RFC v2 07/18] kthread: Allow to cancel kthread work

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 v2 07/18] kthread: Allow to cancel kthread work Petr Mladek <pmladek@suse.com> - 2015-10-07 11:30 +0200
    Re: [RFC v2 07/18] kthread: Allow to cancel kthread work Tejun Heo <tj@kernel.org> - 2015-10-07 16:30 +0200
      Re: [RFC v2 07/18] kthread: Allow to cancel kthread work Petr Mladek <pmladek@suse.com> - 2015-10-14 12:30 +0200
        Re: [RFC v2 07/18] kthread: Allow to cancel kthread work Tejun Heo <tj@kernel.org> - 2015-10-14 19:40 +0200

#1241279 — Re: [RFC v2 07/18] kthread: Allow to cancel kthread work

On Mon 2015-10-05 13:09:24, Petr Mladek wrote:
> On Mon 2015-10-05 12:07:58, Petr Mladek wrote:
> > On Fri 2015-10-02 15:24:53, Tejun Heo wrote:
> > > Hello,
> > > 
> > > On Fri, Oct 02, 2015 at 05:43:36PM +0200, Petr Mladek wrote:
> > > > IMHO, we need both locks. The worker manipulates more works and
> > > > need its own lock. We need work-specific lock because the work
> > > > might be assigned to different workers and we need to be sure
> > > > that the operations are really serialized, e.g. queuing.
> > > 
> > > I don't think we need per-work lock.  Do we have such usage in kernel
> > > at all?  If you're worried, let the first queueing record the worker
> > > and trigger warning if someone tries to queue it anywhere else.  This
> > > doesn't need to be full-on general like workqueue.  Let's make
> > > reasonable trade-offs where possible.
> > 
> > I actually thought about this simplification as well. But then I am
> > in doubts about the API. It would make sense to assign the worker
> > when the work is being initialized and avoid the duplicate information
> > when the work is being queued:
> > 
> > 	init_kthread_work(work, fn, worker);
> > 	queue_work(work);
> > 
> > Or would you prefer to keep the API similar to workqueues even when
> > it makes less sense here?
> > 
> > 
> > In each case, we need a way to switch the worker if the old one
> > is destroyed and a new one is started later. We would need
> > something like:
> > 
> > 	reset_work(work, worker)
> > or
> > 	reinit_work(work, fn, worker)
> 
> I was too fast. We could set "work->worker = NULL" when the work
> finishes and it is not pending. It means that it will be connected
> to the particular worker only when used. Then we could keep the
> workqueues-like API and do not need reset_work().

I have played with this idea and the result is not satisfactory.
I am not able to make the code easier using the single lock.

First, the worker lock is not enough to safely queue the work
without a test_and_set() atomic operation. Let me show this on
a pseudo code:

bool queue_kthread_work(worker, work)
{
	bool ret = false;

	lock(&worker->lock);

	if (test_bit(WORK_PENDING, work->flags);
		goto out;

	if (WARN(work->worker != worker,
		 "Work could not be used by two workers at the same time\n"))
		goto out;

	set_bit(WORK_PENDING, work->flags);
	work->worker = worker;
	insert_work(worker->work_list, work);
	ret = true;

out:
	unlock(&worker->lock);
	return ret;
}

Now, let's have one work: W, two workers: A, B, and try to queue
the same work to the two workers at the same time:

CPU0					CPU1

queue_kthread_work(A, W);		queue_kthread_work(B, W);
  lock(&A->lock);			lock(&B->lock);
  test_bit(WORK_PENDING, W->flags)      test_bit(WORK_PENDING, W->flags)
    # false				  # false
  WARN(W->worker != A);			WARN(W->worker != B);
    # false				  # false

  set_bit(WORK_PENDING, W->flags);	set_bit(WORK_PENDING, W->flags);
  W->worker = A;			W->worker = B;
  insert_work(A->work_list, W);		insert_work(B->work_list, W);

  unlock(&A->lock);			unlock(&B->lock);

=> It is possible and the result is unclear.

We would need to set either WORK_PENDING flag or the work->worker
using a test_and_set atomic operation and bail out if it fails.
But then we are back in the original code.


Second, we still need the busy waiting for the pending timer callback.
Yes, we could set some flag so that the call back does not queue
the work. But cancel_kthread_work_sync() still has to wait.
It could not return if there is still some pending operation
with the struct kthread_work. Otherwise, it never could
be freed a safe way.

Also note that we still need the WORK_PENDING flag. Otherwise, we
would not be able to detect the race when timer is removed but
the callback has not run yet.


Let me to repeat that using per-work and per-worker lock is not an
option either. We would need some crazy hacks to avoid ABBA deadlocks.


All in all, I would prefer to keep the original approach that is
heavily inspired by the workqueues. I think that it is actually
an advantage to reuse some working concept that reinventing wheels.


Best Regards,
Petr
--
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]

#1241552

Hello, Petr.

On Wed, Oct 07, 2015 at 11:21:30AM +0200, Petr Mladek wrote:
> Now, let's have one work: W, two workers: A, B, and try to queue
> the same work to the two workers at the same time:

It's a debug WARN condition to catch silly mistakes.  It can have
minor race conditions.

...
> Second, we still need the busy waiting for the pending timer callback.

Isn't that del_timer_sync()?

> Yes, we could set some flag so that the call back does not queue
> the work. But cancel_kthread_work_sync() still has to wait.
> It could not return if there is still some pending operation
> with the struct kthread_work. Otherwise, it never could
> be freed a safe way.
> 
> Also note that we still need the WORK_PENDING flag. Otherwise, we
> would not be able to detect the race when timer is removed but
> the callback has not run yet.

Yeah, just use a state field as I wrote before.

> Let me to repeat that using per-work and per-worker lock is not an
> option either. We would need some crazy hacks to avoid ABBA deadlocks.
> 
> 
> All in all, I would prefer to keep the original approach that is
> heavily inspired by the workqueues. I think that it is actually
> an advantage to reuse some working concept that reinventing wheels.

At each turn, you come up with non-issues and declare that it needs to
be full workqueue-like implementation but the issues you're raising
seem all rather irrelevant.  Can you please try to take a step back
and put some distance from the implementation details of workqueue?

Thanks.

-- 
tejun
--
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]

#1246496

On Wed 2015-10-07 07:24:46, Tejun Heo wrote:
>  At each turn, you come up with non-issues and declare that it needs to
> be full workqueue-like implementation but the issues you're raising
> seem all rather irrelevant.  Can you please try to take a step back
> and put some distance from the implementation details of workqueue?

JFYI, I do a step back and am trying to convert more kthreads to
the kthread worker API. It helps me to get better insight into
the problematic.

I am still not sure where you see the difference between
workqueues and the kthread worker API. My view is that
the main differences are:

Workqueues			Kthread worker

  + pool of kthreads		  + dedicated kthread

  + kthreads created and	  + kthread created and
    destroyed on demand		    destroyed with the worker

  + can proceed more works	  + one work is proceed at a time
    in parallel from one queue

Otherwise, similar basic set of operations would be useful:

  + create_worker
  + queue_work, queue_delayed_work
  + mod_delayed_work
  + cancel_work, cancel_delayed_work
  + flush_work
  + flush_worker
  + drain_worker
  + destroy_worker

, where queue, mod, cancel operations should work also from IRQ
context.

There are few potentially complicated and sensitive users of the
kthread workers API, e.g. handling nfs callbacks, some kthreads
used for handling network packets, eventually the rcu stuff.
Here the operations need to be secure and rather fast.

IMHO, it would be great if it is easy to convert between the
kthread worker and workqueues API. It will allow to choose
the most effective variant for a given purpose. IMHO, this is
sometimes hard to say without real life testing.

I wonder if I miss some important angle of view.


In each case, it is still not clear if the API will be acceptable
for the affected parties. Therefore I do not want to spend too
much time on perfectionalizing the API implementation at this
point. Is it OK, please?

Thanks for feedback.

Best Regards,
Petr


PS: I am not convinced that all my concerns were non-issues.
For example, I agree that a race when queuing the same work
to more kthread workers might look theoretical. On the other
hand, the API allows it and it might be hard to debug. IMHO,
it might be an acceptable trade off if the implementation is
much easier and more secure in other areas. But my draft
implementation did not suggested this.

For example, there were more situations when I needed to double
check that the work was still connected with the locked worker
after taking the lock. I know that it will not happen when
the API is used a reasonable way but...

Ah, I am back in the details. I have to stop it for now ;-)
--
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]

#1247022

Hello,

On Wed, Oct 14, 2015 at 12:20:22PM +0200, Petr Mladek wrote:
> IMHO, it would be great if it is easy to convert between the
> kthread worker and workqueues API. It will allow to choose

Sure, keep the APIs similar so that they can be easily converted back
and forth but that doesn't mean kthread_worker should be as complex as
workqueue.  Workqueue is *really* complex partly for historical
reasons and partly because it has to serve all corner cases.  Please
make something simple which is similar enough to enable easy miration.
That amount of complexity simply isn't necessary for kthread_worker.

...
> PS: I am not convinced that all my concerns were non-issues.
> For example, I agree that a race when queuing the same work
> to more kthread workers might look theoretical. On the other
> hand, the API allows it and it might be hard to debug. IMHO,

There are big differences in terms of complexity between ensuring
something like the above working correctly under all circumstances and
implementing a warning trap which would trigger well enough to warn
against unsupported usages.  These are active trade-offs to make and
not particularly hard ones either.  Let's please keep kthread_worker
simple.

Thanks.

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