Messages in this thread | | | Date | Thu, 22 Sep 2011 08:55:12 +0530 | Subject | Re: [PATCH 1/1] Trivial: devtmpfsd: Setting task running/interruptible states | From | "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 > -- 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/
| |