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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
