[PATCH 1/1] Trivial: devtmpfsd: Setting task running/interruptible states

[PATCH 1/1] Trivial: devtmpfsd: Setting task running/interruptible states

[Kautuk Consul]

This trivial patch makes the following changes in devtmpfsd() :
- Set the state to TASK_INTERRUPTIBLE using __set_current_state
  instead of set_current_state as the spin_unlock is an implicit
  memory barrier.
- After return from schedule(), there is no need to set the current
  state to TASK_RUNNING as the wake_up_process() function call will
  do this for us.

Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
---
 drivers/base/devtmpfs.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index a4760e0..2bb4bff 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -413,10 +413,9 @@ static int devtmpfsd(void *p)
 			}
 			spin_lock(&req_lock);
 		}
-		set_current_state(TASK_INTERRUPTIBLE);
+		__set_current_state(TASK_INTERRUPTIBLE);
 		spin_unlock(&req_lock);
 		schedule();
-		__set_current_state(TASK_RUNNING);
 	}
 	return 0;
 out:
-- 
1.7.4.1

Re: [PATCH 1/1] Trivial: devtmpfsd: Setting task running/interruptible states

[Greg KH]

On Wed, Sep 21, 2011 at 09:09:33PM +0530, Kautuk Consul wrote:
> This trivial patch makes the following changes in devtmpfsd() :

This is not the definition of "trivial" in that you are changing the
logic of the code, not just doing spelling changes.

> - Set the state to TASK_INTERRUPTIBLE using __set_current_state
>   instead of set_current_state as the spin_unlock is an implicit
>   memory barrier.

Why?  What is this hurting with the original code?

> - After return from schedule(), there is no need to set the current
>   state to TASK_RUNNING as the wake_up_process() function call will
>   do this for us.

Are you sure?

Have you tested this patch and everything works properly?

greg k-h

Re: [PATCH 1/1] Trivial: devtmpfsd: Setting task running/interruptible states

[kautuk.c @samsung.com]

Hi Greg,

On Wed, Sep 21, 2011 at 9:24 PM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Sep 21, 2011 at 09:09:33PM +0530, Kautuk Consul wrote:
>> This trivial patch makes the following changes in devtmpfsd() :
>
> This is not the definition of "trivial" in that you are changing the
> logic of the code, not just doing spelling changes.

Well, I didn't really change the performance/functionality so I called
it trivial.

>
>> - Set the state to TASK_INTERRUPTIBLE using __set_current_state
>>   instead of set_current_state as the spin_unlock is an implicit
>>   memory barrier.
>
> Why?  What is this hurting with the original code?

Nothing really hurting, that's why I called this patch trivial.
There is an extra memory barrier we have to go through by way of
set_current_state, which is mb().
That would lead to more overhead on the parallel pipelines of the processor
as they will have to cease being parallel for instructions before and after
the memory barrier despite the fact that the spin_unlock already covers this.
We can do without this because as per the Documentation/memory-barriers.txt,
atomic operations and unlocks give reliable ordering to instructions.

>
>> - After return from schedule(), there is no need to set the current
>>   state to TASK_RUNNING as the wake_up_process() function call will
>>   do this for us.
>
> Are you sure?

Yes. wake_up_process()->ttwu_queue()->ttwu_do_activate()->ttwu_do_wakeup()
will set the task state to TASK_RUNNING. Before that,
ttwu_activate()->activate_task()->enqueue_task() will have put the task struct
onto the run-queue.

Of course, the rq->lock is held during this which does not allow there
to be any race
conditions between the above process and the task actually waking up on a CPU
and then proceeding to execute without its task state set to TASK_RUNNING.

>
> Have you tested this patch and everything works properly?

Yes. It does so on my system. But I did not try any stress tests.
If you want me to try out any stress test cases I can do so, but I was
convinced after reading the code, documentation and creating a
separate kernel module
with a thread and this kind of state setting behavior and things
seemed fine then in the sense
that the thread did not sleep due to the task state being continued to
be set to
TASK_INTERRUPTIBLE after returning from the schedule() function.

Please correct me if I am wrong in any technical assumptions or any point which
I might have overlooked.

>
> greg k-h
>

Re: [PATCH 1/1] Trivial: devtmpfsd: Setting task running/interruptible states

[Greg KH]

On Wed, Sep 21, 2011 at 09:54:01PM +0530, kautuk.c @samsung.com wrote:
> Hi Greg,
> 
> On Wed, Sep 21, 2011 at 9:24 PM, Greg KH <gregkh@suse.de> wrote:
> > On Wed, Sep 21, 2011 at 09:09:33PM +0530, Kautuk Consul wrote:
> >> This trivial patch makes the following changes in devtmpfsd() :
> >
> > This is not the definition of "trivial" in that you are changing the
> > logic of the code, not just doing spelling changes.
> 
> Well, I didn't really change the performance/functionality so I called
> it trivial.

You changed the code logic, which is not trivial at all in this area.

And actually unneeded from what I can tell, right?

> >
> >> - Set the state to TASK_INTERRUPTIBLE using __set_current_state
> >>   instead of set_current_state as the spin_unlock is an implicit
> >>   memory barrier.
> >
> > Why?  What is this hurting with the original code?
> 
> Nothing really hurting, that's why I called this patch trivial.
> There is an extra memory barrier we have to go through by way of
> set_current_state, which is mb().
> That would lead to more overhead on the parallel pipelines of the processor
> as they will have to cease being parallel for instructions before and after
> the memory barrier despite the fact that the spin_unlock already covers this.
> We can do without this because as per the Documentation/memory-barriers.txt,
> atomic operations and unlocks give reliable ordering to instructions.

But the current code is correct, and not hurting anything, and it's not
on a "fast path" at all, so I'd prefer to keep it as-is and not change
it for the sake of changing it, so I'm not going to accept this patch,
sorry.

greg k-h

Re: [PATCH 1/1] Trivial: devtmpfsd: Setting task running/interruptible states

[kautuk.c @samsung.com]

On Thu, Sep 22, 2011 at 2:40 AM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Sep 21, 2011 at 09:54:01PM +0530, kautuk.c @samsung.com wrote:
>> Hi Greg,
>>
>> On Wed, Sep 21, 2011 at 9:24 PM, Greg KH <gregkh@suse.de> wrote:
>> > On Wed, Sep 21, 2011 at 09:09:33PM +0530, Kautuk Consul wrote:
>> >> This trivial patch makes the following changes in devtmpfsd() :
>> >
>> > This is not the definition of "trivial" in that you are changing the
>> > logic of the code, not just doing spelling changes.
>>
>> Well, I didn't really change the performance/functionality so I called
>> it trivial.
>
> You changed the code logic,

Hmm. Not the code logic of devtmpfsd as such but of the loop involved.

> which is not trivial at all in this area.

Ok.
If you want, maybe I could send another patch for this without marking
it "trivial".

>
> And actually unneeded from what I can tell, right?

Well, there *are* 2 overheads.
As I mentioned, the overheads which I tried to remove by this patch is an extra
memory barrier as well as setting of the task state to TASK_RUNNING.
Of course, they are very minimal and that's why I called this change "trivial".

>
>> >
>> >> - Set the state to TASK_INTERRUPTIBLE using __set_current_state
>> >>   instead of set_current_state as the spin_unlock is an implicit
>> >>   memory barrier.
>> >
>> > Why?  What is this hurting with the original code?
>>
>> Nothing really hurting, that's why I called this patch trivial.
>> There is an extra memory barrier we have to go through by way of
>> set_current_state, which is mb().
>> That would lead to more overhead on the parallel pipelines of the processor
>> as they will have to cease being parallel for instructions before and after
>> the memory barrier despite the fact that the spin_unlock already covers this.
>> We can do without this because as per the Documentation/memory-barriers.txt,
>> atomic operations and unlocks give reliable ordering to instructions.
>
> But the current code is correct, and not hurting anything, and it's not
> on a "fast path" at all, so I'd prefer to keep it as-is and not change
> it for the sake of changing it, so I'm not going to accept this patch,
> sorry.

Ok.
However, I see many changes going in which are purely cosmetic like
restructuring or clean-up of a function, etc.
So this is a category of change that lies (in importance) between a
cosmetic/trivial
change and a minor logic change.
Since this patch is still technically correct, do you mean to say that
this cannot even
be looked as some sort of a "technical" cleanup ?

Also, I see you did not include my comment about the removal of the setting of
TASK_RUNNING. Do you at least accept that ?
If that is so, maybe you could accept the first patch I sent.

Anyway, thanks for the info.

>
> greg k-h
>