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

[Greg KH <gregkh@suse.de>]

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