Re: [RFC v1 1/3] work-simple: Simple work queue implemenation

[Daniel Wagner <daniel.wagner@bmw-carit.de>]

Hi Sebastian,

On 02/13/2015 01:09 PM, Sebastian Andrzej Siewior wrote:
> * Daniel Wagner | 2014-07-11 15:26:11 [+0200]:
> 
>> diff --git a/kernel/sched/work-simple.c b/kernel/sched/work-simple.c
>> --- /dev/null
>> +++ b/kernel/sched/work-simple.c
> …
>> +static int swork_kthread(void *arg)
>> +{
>> +	struct sworker *sw = arg;
>> +	struct swork_event *ev;
>> +
>> +	pr_info("swork_kthread enter\n");
>> +
>> +	for (;;) {
>> +		swait_event_interruptible(sw->wq,
>> +					swork_readable(sw));
>> +		if (kthread_should_stop())
>> +			break;
>> +
>> +		raw_spin_lock(&sw->lock);
> 
> why not the _irq() suffix?

Indeed. That should be with a _irq suffix.

>> +		while (!list_empty(&sw->events)) {
>> +			ev = list_first_entry(&sw->events,
>> +					struct swork_event, list);
>> +			list_del_init(&ev->list);
>> +
>> +			raw_spin_unlock(&sw->lock);
>> +
>> +			ev->func(ev);
>> +			xchg(&ev->flags, 0);
> 
> I've been looking at this for a while and this won't work in longterm.
> Why do you bother using xchg if you don't look at the return value?

Forgot to mention in the commit message (sorry about that), that I was
following what I saw in irq_work.c:__irq_work_run(). Looking at it again
(and reading up a lot on this topic) I agree that doesn't make sense.

> Also, you need to clear that flag _before_ invoking ->func() because
> while that function is beeing executed someone might do another
> queue_work() and expects that that work item invoked again. Atleast this
> is how queue_work() behaves and I would want the same here.

You are right. I tried to simplify what I saw in irq_work and only using
a PENDING flag instead of a PENDING and BUSY flag. Obviously, my version
is broken in that regard. I guess the BUSY flag is necessary.

> But don't worry. I fix this up to me needs so there is nothing you need
> to worry about. The remaining part is simple. Thanks.

Thanks a lot!

cheers,
daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html