From: Tejun Heo <tj@kernel.org> To: Kent Overstreet <koverstreet@google.com> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-aio@kvack.org, akpm@linux-foundation.org, Zach Brown <zab@redhat.com>, Felipe Balbi <balbi@ti.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Mark Fasheh <mfasheh@suse.com>, Joel Becker <jlbec@evilplan.org>, Rusty Russell <rusty@rustcorp.com.au>, Jens Axboe <axboe@kernel.dk>, Asai Thambi S P <asamymuthupa@micron.com>, Selvan Mani <smani@micron.com>, Sam Bradshaw <sbradshaw@micron.com>, Jeff Moyer <jmoyer@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>, Benjamin LaHaise <bcrl@kvack.org>, Oleg Nesterov <oleg@redhat.com>, Christoph Lameter <cl@linux-foundation.org>, Ingo Molnar <mingo@redhat.com> Subject: Re: [PATCH 04/21] Generic percpu refcounting Date: Wed, 15 May 2013 10:56:13 -0700 [thread overview] Message-ID: <20130515175613.GB26222@htj.dyndns.org> (raw) In-Reply-To: <20130515090742.GD16164@moria.home.lan> Hey, On Wed, May 15, 2013 at 02:07:42AM -0700, Kent Overstreet wrote: > > > + __this_cpu_dec(*pcpu_count); > > > + else > > > + ret = atomic_dec_and_test(&ref->count); > > > + > > > + preempt_enable(); > > > + > > > + return ret; > > > > With likely() added, I think the compiler should be able to recognize > > that the branch on pcpu_count should exclude later branch in the > > caller to test for the final put in most cases but I'm a bit worried > > whether that would always be the case and wonder whether ->release > > based interface would be better. Another concern is that the above > > interface is likely to encourage its users to put the release > > implementation in the same function. e.g. > > I... don't follow what you mean hear at all - what exactly would the > compiler do differently? and how would passing a release function > matter? So, on the fast path, there should be one branch on the percpu pointer; however, given the above code, especially without likely(), the compiler may well choose to emit two branches which are shared by both hot and cold paths - the first one on the percpu pointer, the second on whether ref->count reached zero. It just isn't clear to the compiler whether duplicated preempt_enable() or an extra branch would be cheaper. > > void my_put(my_obj) > > { > > if (!percpu_ref_put(&my_obj->ref)) > > return; > > destroy my_obj; > > free my_obj; > > } > > > > Which in turn is likely to nudge the developer or compiler towards not > > inlining the fast path. > > I'm kind of skeptical partial inlining would be worth it for just an > atomic_dec_and_test()... Ooh, you can do the slow path inline too but I *suspect* we probably need a bit more logic in the slowpath anyway if we wanna take care of the bias overflow and maybe the release callback, and it really doesn't matter a bit whether you have a call for slowpath, so... > > So, while I do like the simplicity of put() returning %true on the > > final put, I suspect it's more likely to slowing down fast paths due > > to its interface compared to having separate ->release function > > combined with void put(). Any ideas? > > Oh, you mean having one branch instead of two when we're in percpu mode. > Yeah, that is a good point. Yeap, heh, I should have read to the end before repling. :) > I bet with the likely() added the compiler is going to generate the same > code either way, but I suppose I can have a look at what gcc actually > does... Yeah, with likely(), I *think* gcc should get it right most of the time. There might be some edge cases tho. Thanks. -- tejun
WARNING: multiple messages have this Message-ID (diff)
From: Tejun Heo <tj@kernel.org> To: Kent Overstreet <koverstreet@google.com> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-aio@kvack.org, akpm@linux-foundation.org, Zach Brown <zab@redhat.com>, Felipe Balbi <balbi@ti.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Mark Fasheh <mfasheh@suse.com>, Joel Becker <jlbec@evilplan.org>, Rusty Russell <rusty@rustcorp.com.au>, Jens Axboe <axboe@kernel.dk>, Asai Thambi S P <asamymuthupa@micron.com>, Selvan Mani <smani@micron.com>, Sam Bradshaw <sbradshaw@micron.com>, Jeff Moyer <jmoyer@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>, Benjamin LaHaise <bcrl@kvack.org>, Oleg Nesterov <oleg@redhat.com>, Christoph Lameter <cl@linux-foundation.org>, Ingo Molnar <mingo@redhat.com> Subject: Re: [PATCH 04/21] Generic percpu refcounting Date: Wed, 15 May 2013 10:56:13 -0700 [thread overview] Message-ID: <20130515175613.GB26222@htj.dyndns.org> (raw) In-Reply-To: <20130515090742.GD16164@moria.home.lan> Hey, On Wed, May 15, 2013 at 02:07:42AM -0700, Kent Overstreet wrote: > > > + __this_cpu_dec(*pcpu_count); > > > + else > > > + ret = atomic_dec_and_test(&ref->count); > > > + > > > + preempt_enable(); > > > + > > > + return ret; > > > > With likely() added, I think the compiler should be able to recognize > > that the branch on pcpu_count should exclude later branch in the > > caller to test for the final put in most cases but I'm a bit worried > > whether that would always be the case and wonder whether ->release > > based interface would be better. Another concern is that the above > > interface is likely to encourage its users to put the release > > implementation in the same function. e.g. > > I... don't follow what you mean hear at all - what exactly would the > compiler do differently? and how would passing a release function > matter? So, on the fast path, there should be one branch on the percpu pointer; however, given the above code, especially without likely(), the compiler may well choose to emit two branches which are shared by both hot and cold paths - the first one on the percpu pointer, the second on whether ref->count reached zero. It just isn't clear to the compiler whether duplicated preempt_enable() or an extra branch would be cheaper. > > void my_put(my_obj) > > { > > if (!percpu_ref_put(&my_obj->ref)) > > return; > > destroy my_obj; > > free my_obj; > > } > > > > Which in turn is likely to nudge the developer or compiler towards not > > inlining the fast path. > > I'm kind of skeptical partial inlining would be worth it for just an > atomic_dec_and_test()... Ooh, you can do the slow path inline too but I *suspect* we probably need a bit more logic in the slowpath anyway if we wanna take care of the bias overflow and maybe the release callback, and it really doesn't matter a bit whether you have a call for slowpath, so... > > So, while I do like the simplicity of put() returning %true on the > > final put, I suspect it's more likely to slowing down fast paths due > > to its interface compared to having separate ->release function > > combined with void put(). Any ideas? > > Oh, you mean having one branch instead of two when we're in percpu mode. > Yeah, that is a good point. Yeap, heh, I should have read to the end before repling. :) > I bet with the likely() added the compiler is going to generate the same > code either way, but I suppose I can have a look at what gcc actually > does... Yeah, with likely(), I *think* gcc should get it right most of the time. There might be some edge cases tho. Thanks. -- tejun -- To unsubscribe, send a message with 'unsubscribe linux-aio' in the body to majordomo@kvack.org. For more info on Linux AIO, see: http://www.kvack.org/aio/ Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
next prev parent reply other threads:[~2013-05-15 17:56 UTC|newest] Thread overview: 104+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-05-14 1:18 AIO refactoring/performance improvements/cancellation Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 01/21] aio: fix kioctx not being freed after cancellation at exit time Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 02/21] aio: reqs_active -> reqs_available Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 03/21] aio: percpu reqs_available Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 04/21] Generic percpu refcounting Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 13:51 ` Oleg Nesterov 2013-05-14 13:51 ` Oleg Nesterov 2013-05-15 8:21 ` Kent Overstreet 2013-05-15 8:21 ` Kent Overstreet 2013-05-14 14:59 ` Tejun Heo 2013-05-14 14:59 ` Tejun Heo 2013-05-14 15:28 ` Oleg Nesterov 2013-05-14 15:28 ` Oleg Nesterov 2013-05-15 9:00 ` Kent Overstreet 2013-05-15 9:00 ` Kent Overstreet 2013-05-15 8:58 ` Kent Overstreet 2013-05-15 8:58 ` Kent Overstreet 2013-05-15 17:37 ` Tejun Heo 2013-05-15 17:37 ` Tejun Heo 2013-05-28 23:47 ` Kent Overstreet 2013-05-28 23:47 ` Kent Overstreet 2013-05-29 1:11 ` Tejun Heo 2013-05-29 1:11 ` Tejun Heo 2013-05-29 4:59 ` Rusty Russell 2013-05-29 4:59 ` Rusty Russell 2013-05-31 20:12 ` Kent Overstreet 2013-05-31 20:12 ` Kent Overstreet 2013-05-14 21:59 ` Tejun Heo 2013-05-14 21:59 ` Tejun Heo 2013-05-14 22:15 ` Tejun Heo 2013-05-14 22:15 ` Tejun Heo 2013-05-15 9:07 ` Kent Overstreet 2013-05-15 9:07 ` Kent Overstreet 2013-05-15 17:56 ` Tejun Heo [this message] 2013-05-15 17:56 ` Tejun Heo 2013-05-16 0:26 ` Rusty Russell 2013-05-16 0:26 ` Rusty Russell 2013-05-14 1:18 ` [PATCH 05/21] aio: percpu ioctx refcount Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 06/21] aio: io_cancel() no longer returns the io_event Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 07/21] aio: Don't use ctx->tail unnecessarily Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 08/21] aio: Kill aio_rw_vect_retry() Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 09/21] aio: Kill unneeded kiocb members Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 10/21] aio: Kill ki_users Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 11/21] aio: Kill ki_dtor Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 12/21] aio: convert the ioctx list to radix tree Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 13/21] block: prep work for batch completion Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 14/21] block, aio: batch completion for bios/kiocbs Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 15/21] virtio-blk: convert to batch completion Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 16/21] mtip32xx: " Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 17/21] Percpu tag allocator Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 13:48 ` Oleg Nesterov 2013-05-14 13:48 ` Oleg Nesterov 2013-05-14 14:24 ` Oleg Nesterov 2013-05-14 14:24 ` Oleg Nesterov 2013-05-15 9:34 ` Kent Overstreet 2013-05-15 9:34 ` Kent Overstreet 2013-05-15 9:25 ` Kent Overstreet 2013-05-15 9:25 ` Kent Overstreet 2013-05-15 15:41 ` Oleg Nesterov 2013-05-15 15:41 ` Oleg Nesterov 2013-05-15 16:10 ` Oleg Nesterov 2013-05-15 16:10 ` Oleg Nesterov 2013-06-10 23:20 ` Kent Overstreet 2013-06-10 23:20 ` Kent Overstreet 2013-06-11 17:42 ` Oleg Nesterov 2013-06-11 17:42 ` Oleg Nesterov 2013-05-14 15:03 ` Tejun Heo 2013-05-14 15:03 ` Tejun Heo 2013-05-15 20:19 ` Andi Kleen 2013-05-15 20:19 ` Andi Kleen 2013-05-14 1:18 ` [PATCH 18/21] aio: Allow cancellation without a cancel callback, new kiocb lookup Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 19/21] aio/usb: Update cancellation for new synchonization Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 20/21] direct-io: Set dio->io_error directly Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-14 1:18 ` [PATCH 21/21] block: Bio cancellation Kent Overstreet 2013-05-14 1:18 ` Kent Overstreet 2013-05-15 17:52 ` Jens Axboe 2013-05-15 17:52 ` Jens Axboe 2013-05-15 19:29 ` Kent Overstreet 2013-05-15 19:29 ` Kent Overstreet 2013-05-15 20:01 ` Jens Axboe 2013-05-15 20:01 ` Jens Axboe 2013-05-31 22:52 ` Kent Overstreet 2013-05-31 22:52 ` Kent Overstreet
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20130515175613.GB26222@htj.dyndns.org \ --to=tj@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=asamymuthupa@micron.com \ --cc=axboe@kernel.dk \ --cc=balbi@ti.com \ --cc=bcrl@kvack.org \ --cc=cl@linux-foundation.org \ --cc=gregkh@linuxfoundation.org \ --cc=jlbec@evilplan.org \ --cc=jmoyer@redhat.com \ --cc=koverstreet@google.com \ --cc=linux-aio@kvack.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mfasheh@suse.com \ --cc=mingo@redhat.com \ --cc=oleg@redhat.com \ --cc=rusty@rustcorp.com.au \ --cc=sbradshaw@micron.com \ --cc=smani@micron.com \ --cc=viro@zeniv.linux.org.uk \ --cc=zab@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.