| Message ID | 1353411610-3401-1-git-send-email-sachin.kamat@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On 20/11/12 11:40, Sachin Kamat wrote: > kfree on NULL pointer is a no-op. > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > --- > drivers/staging/ozwpan/ozpd.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c > index 0b3648c..118a4db 100644 > --- a/drivers/staging/ozwpan/ozpd.c > +++ b/drivers/staging/ozwpan/ozpd.c > @@ -402,8 +402,7 @@ static void oz_tx_frame_free(struct oz_pd *pd, struct oz_tx_frame *f) > f = 0; > } > spin_unlock_bh(&pd->tx_frame_lock); > - if (f) > - kfree(f); > + kfree(f); > } > /*------------------------------------------------------------------------------ > * Context: softirq-serialized > @@ -737,8 +736,7 @@ int oz_isoc_stream_create(struct oz_pd *pd, u8 ep_num) > st = 0; > } > spin_unlock_bh(&pd->stream_lock); > - if (st) > - kfree(st); > + kfree(st); > return 0; > } > /*------------------------------------------------------------------------------ Thanks Sachin. However I don't think I can take this patch series for following reasons :- 1. Removing pointer check confuses reader about logic flow. 2. We can save few CPU cycles by checking NULL pointer here, rather than relying on kfree().
On Tue, Nov 20, 2012 at 12:09:57PM +0000, Rupesh Gujare wrote: > 2. We can save few CPU cycles by checking NULL pointer here, rather > than relying on kfree(). I'm not sure about that. But more importantly does any of the memory pools code actually make a difference in benchmarks? What is the difference between running with OZ_MAX_TX_POOL_SIZE as zero and it set to 6? We have this very complicated code with spinlocks and linked lists to save six 56 byte allocations... It's silly. :P Just use kmalloc(). regards, dan carpenter
On 20/11/12 14:16, Dan Carpenter wrote: > But more importantly does any of the memory pools code actually make a > difference in benchmarks? What is the difference between running with > OZ_MAX_TX_POOL_SIZE as zero and it set to 6? Agree. Thats is a good test to look for. > We have this very complicated code with spinlocks and linked lists to > save six 56 byte allocations... It's silly. :P Just use kmalloc(). Probably we will need to kmalloc() & kfree() few hundred times every second when data transfer is taking places. By maintaining a pool of 6, we avoid calling kmalloc frequently. Although I agree that some benchmarking is required here to verify that we actually get some advantage with current implementation.
On Tue, Nov 20, 2012 at 12:09:57PM +0000, Rupesh Gujare wrote: > On 20/11/12 11:40, Sachin Kamat wrote: > >kfree on NULL pointer is a no-op. > > > >Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > >--- > > drivers/staging/ozwpan/ozpd.c | 6 ++---- > > 1 files changed, 2 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c > >index 0b3648c..118a4db 100644 > >--- a/drivers/staging/ozwpan/ozpd.c > >+++ b/drivers/staging/ozwpan/ozpd.c > >@@ -402,8 +402,7 @@ static void oz_tx_frame_free(struct oz_pd *pd, struct oz_tx_frame *f) > > f = 0; > > } > > spin_unlock_bh(&pd->tx_frame_lock); > >- if (f) > >- kfree(f); > >+ kfree(f); > > } > > /*------------------------------------------------------------------------------ > > * Context: softirq-serialized > >@@ -737,8 +736,7 @@ int oz_isoc_stream_create(struct oz_pd *pd, u8 ep_num) > > st = 0; > > } > > spin_unlock_bh(&pd->stream_lock); > >- if (st) > >- kfree(st); > >+ kfree(st); > > return 0; > > } > > /*------------------------------------------------------------------------------ > > Thanks Sachin. However I don't think I can take this patch series > for following reasons :- > > 1. Removing pointer check confuses reader about logic flow. No it doesn't, this is a normal kernel pattern. > 2. We can save few CPU cycles by checking NULL pointer here, rather > than relying on kfree(). Nope, you shouldn't have to worry about this. If this is a fast-path, then you shouldn't be using kfree() in the first place :) thanks, greg k-h
On Tue, Nov 20, 2012 at 04:18:06PM +0000, Rupesh Gujare wrote: > On 20/11/12 14:16, Dan Carpenter wrote: > >But more importantly does any of the memory pools code actually > >make a difference in benchmarks? What is the difference between > >running with OZ_MAX_TX_POOL_SIZE as zero and it set to 6? > > Agree. Thats is a good test to look for. > > >We have this very complicated code with spinlocks and linked lists > >to save six 56 byte allocations... It's silly. :P Just use > >kmalloc(). > > Probably we will need to kmalloc() & kfree() few hundred times every > second when data transfer is taking places. By maintaining a pool of > 6, we avoid calling kmalloc frequently. Although I agree that some > benchmarking is required here to verify that we actually get some > advantage with current implementation. > The kernel already provides a way to handle this. Look at kmem_cache_create(). regards, dan carpenter
On 21/11/12 13:09, Dan Carpenter wrote: > On Tue, Nov 20, 2012 at 04:18:06PM +0000, Rupesh Gujare wrote: >> On 20/11/12 14:16, Dan Carpenter wrote: >>> But more importantly does any of the memory pools code actually >>> make a difference in benchmarks? What is the difference between >>> running with OZ_MAX_TX_POOL_SIZE as zero and it set to 6? >> Agree. Thats is a good test to look for. >> >>> We have this very complicated code with spinlocks and linked lists >>> to save six 56 byte allocations... It's silly. :P Just use >>> kmalloc(). >> Probably we will need to kmalloc() & kfree() few hundred times every >> second when data transfer is taking places. By maintaining a pool of >> 6, we avoid calling kmalloc frequently. Although I agree that some >> benchmarking is required here to verify that we actually get some >> advantage with current implementation. >> > The kernel already provides a way to handle this. Look at > kmem_cache_create(). > > > Thanks Dan, I will have a look at it.
diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c index 0b3648c..118a4db 100644 --- a/drivers/staging/ozwpan/ozpd.c +++ b/drivers/staging/ozwpan/ozpd.c @@ -402,8 +402,7 @@ static void oz_tx_frame_free(struct oz_pd *pd, struct oz_tx_frame *f) f = 0; } spin_unlock_bh(&pd->tx_frame_lock); - if (f) - kfree(f); + kfree(f); } /*------------------------------------------------------------------------------ * Context: softirq-serialized @@ -737,8 +736,7 @@ int oz_isoc_stream_create(struct oz_pd *pd, u8 ep_num) st = 0; } spin_unlock_bh(&pd->stream_lock); - if (st) - kfree(st); + kfree(st); return 0; } /*------------------------------------------------------------------------------
kfree on NULL pointer is a no-op. Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> --- drivers/staging/ozwpan/ozpd.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)