* [PATCH] ipc/mqueue: use correct gfp flags in msg_insert @ 2012-05-14 21:05 Sasha Levin 2012-05-14 23:54 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Sasha Levin @ 2012-05-14 21:05 UTC (permalink / raw) To: dledford, kosaki.motohiro, akpm; +Cc: linux-kernel, Sasha Levin msg_insert() tries to allocate using GFP_KERNEL, while in both cases when it's called, it's coming from an atomic context. Introduced by 7dd7edf ("ipc/mqueue: improve performance of send/recv"). Use GFP_ATOMIC instead. Also, fix up coding style in the kzalloc while we're there. Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- ipc/mqueue.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/ipc/mqueue.c b/ipc/mqueue.c index 30f6f8f..9ec6896 100644 --- a/ipc/mqueue.c +++ b/ipc/mqueue.c @@ -133,7 +133,7 @@ static int msg_insert(struct msg_msg *msg, struct mqueue_inode_info *info) else p = &(*p)->rb_right; } - leaf = kzalloc(sizeof(struct posix_msg_tree_node), GFP_KERNEL); + leaf = kzalloc(sizeof(*leaf), GFP_ATOMIC); if (!leaf) return -ENOMEM; rb_init_node(&leaf->rb_node); -- 1.7.8.6 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ipc/mqueue: use correct gfp flags in msg_insert 2012-05-14 21:05 [PATCH] ipc/mqueue: use correct gfp flags in msg_insert Sasha Levin @ 2012-05-14 23:54 ` Andrew Morton 2012-05-15 2:45 ` Doug Ledford 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2012-05-14 23:54 UTC (permalink / raw) To: Sasha Levin; +Cc: dledford, kosaki.motohiro, linux-kernel On Mon, 14 May 2012 23:05:25 +0200 Sasha Levin <levinsasha928@gmail.com> wrote: > msg_insert() tries to allocate using GFP_KERNEL, while in both cases when it's called, > it's coming from an atomic context. Introduced by 7dd7edf ("ipc/mqueue: improve > performance of send/recv"). > > Use GFP_ATOMIC instead. > > Also, fix up coding style in the kzalloc while we're there. > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > --- > ipc/mqueue.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/ipc/mqueue.c b/ipc/mqueue.c > index 30f6f8f..9ec6896 100644 > --- a/ipc/mqueue.c > +++ b/ipc/mqueue.c > @@ -133,7 +133,7 @@ static int msg_insert(struct msg_msg *msg, struct mqueue_inode_info *info) > else > p = &(*p)->rb_right; > } > - leaf = kzalloc(sizeof(struct posix_msg_tree_node), GFP_KERNEL); > + leaf = kzalloc(sizeof(*leaf), GFP_ATOMIC); > if (!leaf) > return -ENOMEM; > rb_init_node(&leaf->rb_node); hm, that should have spewed warnings everywhere the first time anyone tested it. Doug, is a re-read of Documentation/SubmitChecklist needed? Switching to GFP_ATOMIC is a bit regrettable. Can we avoid this by speculatively allocating the memory before taking the lock, then free it again if we ended up not using it? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipc/mqueue: use correct gfp flags in msg_insert 2012-05-14 23:54 ` Andrew Morton @ 2012-05-15 2:45 ` Doug Ledford 2012-05-15 21:38 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Doug Ledford @ 2012-05-15 2:45 UTC (permalink / raw) To: Andrew Morton; +Cc: Sasha Levin, kosaki.motohiro, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3048 bytes --] On 5/14/2012 7:54 PM, Andrew Morton wrote: > On Mon, 14 May 2012 23:05:25 +0200 > Sasha Levin <levinsasha928@gmail.com> wrote: > >> msg_insert() tries to allocate using GFP_KERNEL, while in both cases when it's called, >> it's coming from an atomic context. Introduced by 7dd7edf ("ipc/mqueue: improve >> performance of send/recv"). >> >> Use GFP_ATOMIC instead. >> >> Also, fix up coding style in the kzalloc while we're there. >> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >> --- >> ipc/mqueue.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/ipc/mqueue.c b/ipc/mqueue.c >> index 30f6f8f..9ec6896 100644 >> --- a/ipc/mqueue.c >> +++ b/ipc/mqueue.c >> @@ -133,7 +133,7 @@ static int msg_insert(struct msg_msg *msg, struct mqueue_inode_info *info) >> else >> p = &(*p)->rb_right; >> } >> - leaf = kzalloc(sizeof(struct posix_msg_tree_node), GFP_KERNEL); >> + leaf = kzalloc(sizeof(*leaf), GFP_ATOMIC); >> if (!leaf) >> return -ENOMEM; >> rb_init_node(&leaf->rb_node); > > hm, that should have spewed warnings everywhere the first time anyone > tested it. Doug, is a re-read of Documentation/SubmitChecklist needed? Re-read? I never it read it a first time, so hard for me to re-read it. But thanks for pointing it out. Now I've read it. > Switching to GFP_ATOMIC is a bit regrettable. Can we avoid this by > speculatively allocating the memory before taking the lock, then free > it again if we ended up not using it? Not really, we take the lock in a different function than this and would have to pass around a node struct and then free it if we didn't use it. I mean, it could be done, but it would fugly the calls around this up. The msg_insert() routine is called in two places. In one place, the lock is taken right there so you could allocate before and then call. In the other, it is another function called with the lock held so now you would have to pass the possible mem allocation around two functions. Doable, but ugly. On the other hand, this is a small struct that should be coming off one of the small size kmem cache pools (4 pointers total, a long, and an int, so kmalloc-32 or kmalloc-64 depending on arch). That doesn't seem like a likely candidate to fail if there is memory pressure, especially considering that immediately prior to taking the lock we call kmalloc with GFP_KERNEL (as part of load_msg()) and so we should either not be under serious memory pressure or we would have slept waiting for it to ease up. I think I can imagine a better way to do this though as part of the whole request to cache at least one rbnode entry so we get the 0 message performance of the queue back. I'll send that patch through once I've verified it does what I think it will. -- Doug Ledford <dledford@redhat.com> GPG KeyID: 0E572FDD http://people.redhat.com/dledford Infiniband specific RPMs available at http://people.redhat.com/dledford/Infiniband [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 898 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipc/mqueue: use correct gfp flags in msg_insert 2012-05-15 2:45 ` Doug Ledford @ 2012-05-15 21:38 ` Andrew Morton 2012-05-16 4:37 ` Doug Ledford 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2012-05-15 21:38 UTC (permalink / raw) To: Doug Ledford; +Cc: Sasha Levin, kosaki.motohiro, linux-kernel On Mon, 14 May 2012 22:45:57 -0400 Doug Ledford <dledford@redhat.com> wrote: > On 5/14/2012 7:54 PM, Andrew Morton wrote: > > On Mon, 14 May 2012 23:05:25 +0200 > > Sasha Levin <levinsasha928@gmail.com> wrote: > > > >> msg_insert() tries to allocate using GFP_KERNEL, while in both cases when it's called, > >> it's coming from an atomic context. Introduced by 7dd7edf ("ipc/mqueue: improve > >> performance of send/recv"). > >> > >> Use GFP_ATOMIC instead. > >> > >> Also, fix up coding style in the kzalloc while we're there. > >> > >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >> --- > >> ipc/mqueue.c | 2 +- > >> 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/ipc/mqueue.c b/ipc/mqueue.c > >> index 30f6f8f..9ec6896 100644 > >> --- a/ipc/mqueue.c > >> +++ b/ipc/mqueue.c > >> @@ -133,7 +133,7 @@ static int msg_insert(struct msg_msg *msg, struct mqueue_inode_info *info) > >> else > >> p = &(*p)->rb_right; > >> } > >> - leaf = kzalloc(sizeof(struct posix_msg_tree_node), GFP_KERNEL); > >> + leaf = kzalloc(sizeof(*leaf), GFP_ATOMIC); > >> if (!leaf) > >> return -ENOMEM; > >> rb_init_node(&leaf->rb_node); > > > > hm, that should have spewed warnings everywhere the first time anyone > > tested it. Doug, is a re-read of Documentation/SubmitChecklist needed? > > Re-read? I never it read it a first time, so hard for me to re-read it. > But thanks for pointing it out. Now I've read it. I was being nice ;) > > Switching to GFP_ATOMIC is a bit regrettable. Can we avoid this by > > speculatively allocating the memory before taking the lock, then free > > it again if we ended up not using it? > > Not really, we take the lock in a different function than this and would > have to pass around a node struct and then free it if we didn't use it. > I mean, it could be done, but it would fugly the calls around this up. > The msg_insert() routine is called in two places. In one place, the > lock is taken right there so you could allocate before and then call. > In the other, it is another function called with the lock held so now > you would have to pass the possible mem allocation around two functions. > Doable, but ugly. Well, it's not *too* bad: just pass a void** around, zero it if it got used, then unconditionally free it. Nice and easy. But in a contest between source-level beauty and runtime robustness, robustness surely wins? > On the other hand, this is a small struct that > should be coming off one of the small size kmem cache pools (4 pointers > total, a long, and an int, so kmalloc-32 or kmalloc-64 depending on > arch). That doesn't seem like a likely candidate to fail if there is > memory pressure, especially considering that immediately prior to taking > the lock we call kmalloc with GFP_KERNEL (as part of load_msg()) and so > we should either not be under serious memory pressure or we would have > slept waiting for it to ease up. True. But weaselly! > I think I can imagine a better way to do this though as part of the > whole request to cache at least one rbnode entry so we get the 0 message > performance of the queue back. I'll send that patch through once I've > verified it does what I think it will. That sounds good. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipc/mqueue: use correct gfp flags in msg_insert 2012-05-15 21:38 ` Andrew Morton @ 2012-05-16 4:37 ` Doug Ledford 0 siblings, 0 replies; 5+ messages in thread From: Doug Ledford @ 2012-05-16 4:37 UTC (permalink / raw) To: Andrew Morton; +Cc: Sasha Levin, kosaki.motohiro, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2798 bytes --] On 5/15/2012 5:38 PM, Andrew Morton wrote: > On Mon, 14 May 2012 22:45:57 -0400 > Doug Ledford <dledford@redhat.com> wrote: >>> Switching to GFP_ATOMIC is a bit regrettable. Can we avoid this by >>> speculatively allocating the memory before taking the lock, then free >>> it again if we ended up not using it? >> >> Not really, we take the lock in a different function than this and would >> have to pass around a node struct and then free it if we didn't use it. >> I mean, it could be done, but it would fugly the calls around this up. >> The msg_insert() routine is called in two places. In one place, the >> lock is taken right there so you could allocate before and then call. >> In the other, it is another function called with the lock held so now >> you would have to pass the possible mem allocation around two functions. >> Doable, but ugly. > > Well, it's not *too* bad: just pass a void** around, zero it if it got > used, then unconditionally free it. Nice and easy. > > But in a contest between source-level beauty and runtime robustness, > robustness surely wins? Sure, things must be robustness. It's just a question of what qualifies as robust. >> On the other hand, this is a small struct that >> should be coming off one of the small size kmem cache pools (4 pointers >> total, a long, and an int, so kmalloc-32 or kmalloc-64 depending on >> arch). That doesn't seem like a likely candidate to fail if there is >> memory pressure, especially considering that immediately prior to taking >> the lock we call kmalloc with GFP_KERNEL (as part of load_msg()) and so >> we should either not be under serious memory pressure or we would have >> slept waiting for it to ease up. > > True. But weaselly! And we don't rely on weaselly things in the kernel all the time? ;-) >> I think I can imagine a better way to do this though as part of the >> whole request to cache at least one rbnode entry so we get the 0 message >> performance of the queue back. I'll send that patch through once I've >> verified it does what I think it will. > > That sounds good. I coded something up, but I'm not happy with it. Trying again. One of my problems is I *really* don't like speculatively allocating this little struct because in the common case this is a total waste of CPU cycles. While my patch sped up operations under any sort of decent queue depth, it slowed down the queue empty case by 100ns. I'm trying to get some of that loss back, and doing a useless speculative allocation doesn't help :-/ -- Doug Ledford <dledford@redhat.com> GPG KeyID: 0E572FDD http://people.redhat.com/dledford Infiniband specific RPMs available at http://people.redhat.com/dledford/Infiniband [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 898 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-16 4:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-05-14 21:05 [PATCH] ipc/mqueue: use correct gfp flags in msg_insert Sasha Levin 2012-05-14 23:54 ` Andrew Morton 2012-05-15 2:45 ` Doug Ledford 2012-05-15 21:38 ` Andrew Morton 2012-05-16 4:37 ` Doug Ledford
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).