Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1253895 > unrolled thread

Re: [PATCH] mutex: make mutex_lock_nested an inline function

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mutex: make mutex_lock_nested an inline function Arnd Bergmann <arnd@arndb.de> - 2015-10-22 17:10 +0200
    Re: [PATCH] mutex: make mutex_lock_nested an inline function Peter Zijlstra <peterz@infradead.org> - 2015-10-22 17:20 +0200
      Re: [PATCH] mutex: make mutex_lock_nested an inline function Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-10-22 19:50 +0200
        Re: [PATCH] mutex: make mutex_lock_nested an inline function Ingo Molnar <mingo@kernel.org> - 2015-10-27 19:20 +0100

#1253895 — Re: [PATCH] mutex: make mutex_lock_nested an inline function

On Tuesday 13 October 2015 23:46:35 Arnd Bergmann wrote:
> On Tuesday 13 October 2015 22:38:12 Peter Zijlstra wrote:
> > A quick grep shows a large amount of trivial code that optimizers will
> > still happily throw away, but it should be verified that this does not
> > result in pointless code generation.
> 
> Indeed, I'm seeing a tiny code growth with ARM multi_v7_defconfig when
> my patch is applied, as the image (according to size -A) grows from
> 13740187 bytes to 13740283, all of it in .text of two drivers (i2c-core
> and three files of bluetooth.ko).


Did we actually reach any conclusion here? We still get the warnings
in the regulator code in linux-next, and I'd like to see either this
patch ("mutex: make mutex_lock_nested an inline function") or
"regulator: core: avoid unused variable warning" get merged.

We could also remove the two functions again, as they are still
unused and we are getting closer to the merge window.

	Arnd

> --- build/multi_v7_defconfig-before/vmlinux.o.size	2015-10-13 23:11:40.544389776 +0200
> +++ build/multi_v7_defconfig/vmlinux.o.size	2015-10-13 23:08:00.151043811 +0200
> @@ -1,6 +1,6 @@
>  build/multi_v7_defconfig/vmlinux.o  :
>  section                                                          size   addr
> -.text                                                         8219408      0
> +.text                                                         8219504      0
>  
> --- build/multi_v7_defconfig-before/net/bluetooth/bluetooth.ko.size	2015-10-13 23:11:40.704382038 +0200
> +++ build/multi_v7_defconfig/net/bluetooth/bluetooth.ko.size	2015-10-13 23:07:58.639116862 +0200
> @@ -1,7 +1,7 @@
>  build/multi_v7_defconfig/net/bluetooth/bluetooth.ko  :
>  section                       size   addr
>  .note.gnu.build-id              36      0
> -.text                       241512      0
> +.text                       241696      0
>  
> --- build/multi_v7_defconfig-before/drivers/i2c/i2c-core.o.size	2015-10-13 23:11:40.636385326 +0200
> +++ build/multi_v7_defconfig/drivers/i2c/i2c-core.o.size	2015-10-13 23:07:53.403369830 +0200
> @@ -1,6 +1,6 @@
>  build/multi_v7_defconfig/drivers/i2c/i2c-core.o  :
>  section                                                 size   addr
> -.text                                                  12112      0
> +.text                                                  12208      0
> 
> The code in question is 
> 
> a)
> 
> static ssize_t
> i2c_sysfs_delete_device(struct device *dev, struct device_attribute *attr,
>                         const char *buf, size_t count)
> {
> ...
>         mutex_lock_nested(&adap->userspace_clients_lock,
>                           i2c_adapter_depth(adap));
> ...
> }
> 
> and
> 
> b)
> 
> static inline void l2cap_chan_lock(struct l2cap_chan *chan)
> {                             
>         mutex_lock_nested(&chan->lock, atomic_read(&chan->nesting));
> }       
> 
> The first one has a small size impact but no performance change as it is only
> called during probe/release of i2c modules. The second one adds an extra
> pointer access (due to the volatile keyword in atomic_read()) for every
> caller of l2cap_chan_lock().
> 

--
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/

[toc] | [next] | [standalone]

#1253899

On Thu, Oct 22, 2015 at 05:02:45PM +0200, Arnd Bergmann wrote:
> On Tuesday 13 October 2015 23:46:35 Arnd Bergmann wrote:
> > On Tuesday 13 October 2015 22:38:12 Peter Zijlstra wrote:
> > > A quick grep shows a large amount of trivial code that optimizers will
> > > still happily throw away, but it should be verified that this does not
> > > result in pointless code generation.
> > 
> > Indeed, I'm seeing a tiny code growth with ARM multi_v7_defconfig when
> > my patch is applied, as the image (according to size -A) grows from
> > 13740187 bytes to 13740283, all of it in .text of two drivers (i2c-core
> > and three files of bluetooth.ko).
> 
> 
> Did we actually reach any conclusion here? We still get the warnings
> in the regulator code in linux-next, and I'd like to see either this
> patch ("mutex: make mutex_lock_nested an inline function") or
> "regulator: core: avoid unused variable warning" get merged.
> 
> We could also remove the two functions again, as they are still
> unused and we are getting closer to the merge window.

Hmm, I was sure I send a reply, but I cannot even find it in my own sent
folder so who knows.

My current preference is to keep the thing a macro and work around it in
the usage site because while these warns are annoying, they're at least
visible. Whereas, with an inline, code bloat is entirely silent. Even if
the sites you found are harmless, there's no saying what the future will
bring etc..
--
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/

[toc] | [prev] | [next] | [standalone]

#1254013

On Thu, Oct 22, 2015 at 05:09:59PM +0200, Peter Zijlstra wrote:
> Hmm, I was sure I send a reply, but I cannot even find it in my own sent
> folder so who knows.
> 
> My current preference is to keep the thing a macro and work around it in
> the usage site because while these warns are annoying, they're at least
> visible. Whereas, with an inline, code bloat is entirely silent. Even if
> the sites you found are harmless, there's no saying what the future will
> bring etc..

I agree - we've got way too many inline functions already.  My biggest
annoyance in that respect is the asm-generic dma_map_single()
implementation that we're now forced to use on ARM, which results in
quite a large chunk of code at every callsite.

The problem there is that when you have drivers which do something like:

	dma = dma_map_single(dev, page_address(page), size, dir);

you end up with code which converts the struct page to a virtual address,
and then you end up with code in the dma_map_single() inline function
which then converts it back to a struct page + offset - none of which,
with modern ARM kernels, the compiler has a hope in hell of optimising.

So we end up with all that junk at every single dma_map_single() callsite.
If dma_map_single() were a library function, it would be a lot smaller
since we'd only have one copy of the complex virt->struct page conversion.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
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/

[toc] | [prev] | [next] | [standalone]

#1257108

* Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Oct 22, 2015 at 05:09:59PM +0200, Peter Zijlstra wrote:
> > Hmm, I was sure I send a reply, but I cannot even find it in my own sent
> > folder so who knows.
> > 
> > My current preference is to keep the thing a macro and work around it in
> > the usage site because while these warns are annoying, they're at least
> > visible. Whereas, with an inline, code bloat is entirely silent. Even if
> > the sites you found are harmless, there's no saying what the future will
> > bring etc..
> 
> I agree - we've got way too many inline functions already.  My biggest annoyance 
> in that respect is the asm-generic dma_map_single() implementation that we're 
> now forced to use on ARM, which results in quite a large chunk of code at every 
> callsite.
> 
> The problem there is that when you have drivers which do something like:
> 
> 	dma = dma_map_single(dev, page_address(page), size, dir);
> 
> you end up with code which converts the struct page to a virtual address, and 
> then you end up with code in the dma_map_single() inline function which then 
> converts it back to a struct page + offset - none of which, with modern ARM 
> kernels, the compiler has a hope in hell of optimising.
> 
> So we end up with all that junk at every single dma_map_single() callsite. If 
> dma_map_single() were a library function, it would be a lot smaller since we'd 
> only have one copy of the complex virt->struct page conversion.

Should be pretty easy to fix, once you know which inline functions hurt.

Thanks,

	Ingo
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web