From: Matt H. <mat...@us...> - 2006-04-29 01:25:10
|
On 2006-04-27 07:57 Jes Sorensen <je...@sg...> wrote: <snip> A portion of this patch is a kmalloc/memset cleanup that can go in regardless of the task notifier chain. > + size = sizeof(struct notifier_block); > + nb = (struct notifier_block *)kzalloc(size, GFP_KERNEL); I think this could be simplified to: nb = kzalloc(sizeof(*nb), GFP_KERNEL); > Index: linux-2.6/kernel/exit.c > =================================================================== > --- linux-2.6.orig/kernel/exit.c > +++ linux-2.6/kernel/exit.c > @@ -917,7 +917,6 @@ > raw_notifier_call_chain(&tsk->task_notifier, TN_EXIT, tsk); > exit_mm(tsk); > > - exit_sem(tsk); So moving exit_sem(tsk) before exit_mm(tsk) does not introduce any bugs? I think it's important to establish that the patches do not introduce bugs due to ordering assumptions between the original call site and the notifier chain invocation. Using notifiers in these paths seems like an improvement. However, I think we need to see more than 1 user (perhaps 3-4) of these chains before we can be assured they are properly located. You mentioned some additional patches you had been working on... Cheers, -Matt Helsley |
From: Jes S. <je...@sg...> - 2006-05-01 09:38:51
|
Matt Helsley wrote: > On 2006-04-27 07:57 Jes Sorensen <je...@sg...> wrote: > <snip> > > A portion of this patch is a kmalloc/memset cleanup that can go in > regardless of the task notifier chain. > >> + size = sizeof(struct notifier_block); >> + nb = (struct notifier_block *)kzalloc(size, GFP_KERNEL); > > I think this could be simplified to: > > nb = kzalloc(sizeof(*nb), GFP_KERNEL); Hi Matt, I think this falls within the personal preference section. Both styles are well within CodingStyle. I prefer the former as it's more explicit. >> Index: linux-2.6/kernel/exit.c >> =================================================================== >> --- linux-2.6.orig/kernel/exit.c >> +++ linux-2.6/kernel/exit.c >> @@ -917,7 +917,6 @@ >> raw_notifier_call_chain(&tsk->task_notifier, TN_EXIT, tsk); >> exit_mm(tsk); >> >> - exit_sem(tsk); > > So moving exit_sem(tsk) before exit_mm(tsk) does not introduce any bugs? > > I think it's important to establish that the patches do not introduce > bugs due to ordering assumptions between the original call site and the > notifier chain invocation. Yes, but I agree we have to be careful. For the sysv semaphore it's purely a cleanup of something that may or may not be kmalloc'ed and is pointed to from the task struct. > Using notifiers in these paths seems like an improvement. However, I > think we need to see more than 1 user (perhaps 3-4) of these chains > before we can be assured they are properly located. You mentioned some > additional patches you had been working on... I agree, I posted the semundo stuff because I had that one ready, but I think there are a couple of other obvious candidates to add. Cheers, Jes |
From: Matt H. <mat...@us...> - 2006-05-01 21:50:39
|
On Mon, 2006-05-01 at 11:38 +0200, Jes Sorensen wrote: > Matt Helsley wrote: > > On 2006-04-27 07:57 Jes Sorensen <je...@sg...> wrote: > > <snip> > > > > A portion of this patch is a kmalloc/memset cleanup that can go in > > regardless of the task notifier chain. > > > >> + size = sizeof(struct notifier_block); > >> + nb = (struct notifier_block *)kzalloc(size, GFP_KERNEL); > > > > I think this could be simplified to: > > > > nb = kzalloc(sizeof(*nb), GFP_KERNEL); > > Hi Matt, > > I think this falls within the personal preference section. Both styles > are well within CodingStyle. I prefer the former as it's more explicit. I think there are good reasons to choose this form. It adapts to any future change in the struct name for nb. It also preserves the value of the size variable, which makes the patch easier to review since you don't have to check if size is used later in the function. Otherwise yes, it seems to be a matter of personal style. <snip> Cheers, -Matt Helsley |
From: Jes S. <je...@sg...> - 2006-05-02 08:11:18
|
Matt Helsley wrote: > On Mon, 2006-05-01 at 11:38 +0200, Jes Sorensen wrote: >> I think this falls within the personal preference section. Both styles >> are well within CodingStyle. I prefer the former as it's more explicit. > > I think there are good reasons to choose this form. It adapts to any > future change in the struct name for nb. It also preserves the value of > the size variable, which makes the patch easier to review since you > don't have to check if size is used later in the function. Likewise I think there are good reasons for chosing the other form, I have had quite a few cases chasing bugs that ended up being because someone forgot to add the '*' in the sizeof argument. The other way that is hard to get wrong :) I'd prefer to keep it as is. Cheers, Jes |