[PATCH] drivers/infiniband/hw/nes/nes_verbs.c: Deinline nes_free_qp_mem, save 1072 bytes

[PATCH] drivers/infiniband/hw/nes/nes_verbs.c: Deinline nes_free_qp_mem, save 1072 bytes

[Denys Vlasenko]

This function compiles to 550 bytes of machine code.
Three callsites, all in nes_create_qp.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Faisal Latif <faisal.latif@intel.com>
CC: Doug Ledford <dledford@redhat.com>
CC: linux-rdma@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/infiniband/hw/nes/nes_verbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/nes/nes_verbs.c b/drivers/infiniband/hw/nes/nes_verbs.c
index fba69a3..5f48d08 100644
--- a/drivers/infiniband/hw/nes/nes_verbs.c
+++ b/drivers/infiniband/hw/nes/nes_verbs.c
@@ -981,7 +981,7 @@ static int nes_setup_mmap_qp(struct nes_qp *nesqp, struct nes_vnic *nesvnic,
 /**
  * nes_free_qp_mem() is to free up the qp's pci_alloc_consistent() memory.
  */
-static inline void nes_free_qp_mem(struct nes_device *nesdev,
+static void nes_free_qp_mem(struct nes_device *nesdev,
 		struct nes_qp *nesqp, int virt_wqs)
 {
 	unsigned long flags;
-- 
2.1.0

[PATCH] drivers/scsi/fnic/fnic_scsi.c: Deinline fnic_queue_abort_io_req, save 1792 bytes

[Denys Vlasenko]

This function compiles to 511 bytes of machine code.

Abort commands are not time-critical at all.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: James Bottomley <James.Bottomley@hansenpartnership.com>
CC: Hiral Patel <hiralpat@cisco.com>
CC: Suma Ramars <sramars@cisco.com>
CC: Brian Uchino <buchino@cisco.com>
CC: linux-scsi@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/scsi/fnic/fnic_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 266b909..0a3edee 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -1435,7 +1435,7 @@ wq_copy_cleanup_scsi_cmd:
 	}
 }
 
-static inline int fnic_queue_abort_io_req(struct fnic *fnic, int tag,
+static int fnic_queue_abort_io_req(struct fnic *fnic, int tag,
 					  u32 task_req, u8 *fc_lun,
 					  struct fnic_io_req *io_req)
 {
-- 
2.1.0

Re: [PATCH] drivers/scsi/fnic/fnic_scsi.c: Deinline fnic_queue_abort_io_req, save 1792 bytes

[Laurence Oberman]

Simple change, looks fine to me.

Reviewed-by: Laurence Oberman <loberman@redhat.com>

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Denys Vlasenko" <dvlasenk@redhat.com>
To: "James Bottomley" <James.Bottomley@hansenpartnership.com>
Cc: "Denys Vlasenko" <dvlasenk@redhat.com>, "Hiral Patel" <hiralpat@cisco.com>, "Suma Ramars" <sramars@cisco.com>, "Brian Uchino" <buchino@cisco.com>, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Sent: Friday, April 8, 2016 2:58:43 PM
Subject: [PATCH] drivers/scsi/fnic/fnic_scsi.c: Deinline fnic_queue_abort_io_req, save 1792 bytes

This function compiles to 511 bytes of machine code.

Abort commands are not time-critical at all.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: James Bottomley <James.Bottomley@hansenpartnership.com>
CC: Hiral Patel <hiralpat@cisco.com>
CC: Suma Ramars <sramars@cisco.com>
CC: Brian Uchino <buchino@cisco.com>
CC: linux-scsi@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/scsi/fnic/fnic_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 266b909..0a3edee 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -1435,7 +1435,7 @@ wq_copy_cleanup_scsi_cmd:
 	}
 }
 
-static inline int fnic_queue_abort_io_req(struct fnic *fnic, int tag,
+static int fnic_queue_abort_io_req(struct fnic *fnic, int tag,
 					  u32 task_req, u8 *fc_lun,
 					  struct fnic_io_req *io_req)
 {
-- 
2.1.0

Re: [PATCH] drivers/scsi/fnic/fnic_scsi.c: Deinline fnic_queue_abort_io_req, save 1792 bytes

[Martin K. Petersen]

>>>>> "Denys" == Denys Vlasenko <dvlasenk@redhat.com> writes:

Denys> This function compiles to 511 bytes of machine code.  Abort
Denys> commands are not time-critical at all.

Satish, please review.

https://patchwork.kernel.org/patch/8785281/

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] drivers/virtio/virtio_ring.c: Deinline virtqueue_add, save 1016 bytes

[Denys Vlasenko]

This function compiles to 839 bytes of machine code.
In C, it is ~150 lines long.

This function has 3 callsites.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: virtualization@lists.linux-foundation.org
CC: linux-kernel@vger.kernel.org
---
 drivers/virtio/virtio_ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e12e385..77a4771 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -126,7 +126,7 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
 	return desc;
 }
 
-static inline int virtqueue_add(struct virtqueue *_vq,
+static int virtqueue_add(struct virtqueue *_vq,
 				struct scatterlist *sgs[],
 				unsigned int total_sg,
 				unsigned int out_sgs,
-- 
2.1.0

Re: [PATCH] drivers/virtio/virtio_ring.c: Deinline virtqueue_add, save 1016 bytes

[Michael S. Tsirkin]

On Fri, Apr 08, 2016 at 08:58:44PM +0200, Denys Vlasenko wrote:
> This function compiles to 839 bytes of machine code.
> In C, it is ~150 lines long.
> 
> This function has 3 callsites.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: virtualization@lists.linux-foundation.org
> CC: linux-kernel@vger.kernel.org

This function is one of the most performance critical ones in the driver, a
bunch of tuning went into it, making this inline intentionally.  I'd
have to see some numbers showing making it non-inline is a worth-while
tradeoff.

> ---
>  drivers/virtio/virtio_ring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e12e385..77a4771 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -126,7 +126,7 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
>  	return desc;
>  }
>  
> -static inline int virtqueue_add(struct virtqueue *_vq,
> +static int virtqueue_add(struct virtqueue *_vq,
>  				struct scatterlist *sgs[],
>  				unsigned int total_sg,
>  				unsigned int out_sgs,
> -- 
> 2.1.0

[PATCH] fs/gfs2/glock.c: Deinline do_error, save 1856 bytes

[Denys Vlasenko]

This function compiles to 522 bytes of machine code.

Error paths are not very time critical.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Steven Whitehouse <swhiteho@redhat.com>
CC: Bob Peterson <rpeterso@redhat.com>
CC: cluster-devel@redhat.com
CC: linux-kernel@vger.kernel.org
---
 fs/gfs2/glock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 6539131..c3d5172 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -218,7 +218,7 @@ static void gfs2_holder_wake(struct gfs2_holder *gh)
  *
  */
 
-static inline void do_error(struct gfs2_glock *gl, const int ret)
+static void do_error(struct gfs2_glock *gl, const int ret)
 {
 	struct gfs2_holder *gh, *tmp;
 
-- 
2.1.0

Re: [PATCH] fs/gfs2/glock.c: Deinline do_error, save 1856 bytes

[Bob Peterson]

----- Original Message -----
> This function compiles to 522 bytes of machine code.
> 
> Error paths are not very time critical.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Steven Whitehouse <swhiteho@redhat.com>
> CC: Bob Peterson <rpeterso@redhat.com>
> CC: cluster-devel@redhat.com
> CC: linux-kernel@vger.kernel.org
> ---
>  fs/gfs2/glock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 6539131..c3d5172 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -218,7 +218,7 @@ static void gfs2_holder_wake(struct gfs2_holder *gh)
>   *
>   */
>  
> -static inline void do_error(struct gfs2_glock *gl, const int ret)
> +static void do_error(struct gfs2_glock *gl, const int ret)
>  {
>  	struct gfs2_holder *gh, *tmp;
>  
> --
> 2.1.0
> 
> 

Hi Denys,

The name is misleading. Function do_error() isn't really an error path.
Its job is to "fail" all the holders for a glock that are doing a "try" lock
in cases where trying the lock has been determined to have failed.

Is there a reason why you want to trade memory for speed? Are you
optimizing for memory on an embedded device or something?
I guess I have no fundamental problem in adding this patch, but perhaps
Steve or someone can offer a second opinion before I do.

Regards,

Bob Peterson
Red Hat File Systems

Re: [PATCH] fs/gfs2/glock.c: Deinline do_error, save 1856 bytes

[Denys Vlasenko]

On 04/08/2016 09:17 PM, Bob Peterson wrote:
> ----- Original Message -----
>> This function compiles to 522 bytes of machine code.
>>
> Is there a reason why you want to trade memory for speed? Are you
> optimizing for memory on an embedded device or something?

Yes. I did a scan for really large inlines and this function came up.
If you feel it is indeed performance critical, please ignore my patch.

Re: [PATCH] fs/gfs2/glock.c: Deinline do_error, save 1856 bytes

[Bob Peterson]

----- Original Message -----
> This function compiles to 522 bytes of machine code.
> 
> Error paths are not very time critical.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
> CC: Steven Whitehouse <swhiteho@redhat.com>
> CC: Bob Peterson <rpeterso@redhat.com>
> CC: cluster-devel@redhat.com
> CC: linux-kernel@vger.kernel.org
> ---
>  fs/gfs2/glock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 6539131..c3d5172 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -218,7 +218,7 @@ static void gfs2_holder_wake(struct gfs2_holder *gh)
>   *
>   */
>  
> -static inline void do_error(struct gfs2_glock *gl, const int ret)
> +static void do_error(struct gfs2_glock *gl, const int ret)
>  {
>  	struct gfs2_holder *gh, *tmp;
>  
> --
> 2.1.0
> 
> 

Hi,

Thanks. This is now applied to the for-next branch of the linux-gfs2 tree:
https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?h=for-next&id=a527b38e1475211b67eb59b3fadb40689f035529

Regards,

Bob Peterson
Red Hat File Systems

[PATCH] lockdep: Deinline register_lock_class, save 2328 bytes

[Denys Vlasenko]

This function compiles to 1328 bytes of machine code. Three callsites.

Registering a new lock class is definitely not *that* time-critical to inline it.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Ingo Molnar <mingo@redhat.com>
CC: linux-kernel@vger.kernel.org
---
 kernel/locking/lockdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 53ab2f8..ffad373 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -708,7 +708,7 @@ look_up_lock_class(struct lockdep_map *lock, unsigned int subclass)
  * yet. Otherwise we look it up. We cache the result in the lock object
  * itself, so actual lookup of the hash should be once per lock object.
  */
-static inline struct lock_class *
+static struct lock_class *
 register_lock_class(struct lockdep_map *lock, unsigned int subclass, int force)
 {
 	struct lockdep_subclass_key *key;
-- 
2.1.0

[tip:locking/core] locking/lockdep: Deinline register_lock_class(), save 2328 bytes

[tip-bot for Denys Vlasenko]

Commit-ID:  c003ed928962a55eb446e78c544b1d7c4f6cb88a
Gitweb:     http://git.kernel.org/tip/c003ed928962a55eb446e78c544b1d7c4f6cb88a
Author:     Denys Vlasenko <dvlasenk@redhat.com>
AuthorDate: Fri, 8 Apr 2016 20:58:46 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 13 Apr 2016 10:06:13 +0200

locking/lockdep: Deinline register_lock_class(), save 2328 bytes

This function compiles to 1328 bytes of machine code. Three callsites.

Registering a new lock class is definitely not *that* time-critical to inline it.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/1460141926-13069-5-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/lockdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index ed94109..7cc43ef 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -708,7 +708,7 @@ look_up_lock_class(struct lockdep_map *lock, unsigned int subclass)
  * yet. Otherwise we look it up. We cache the result in the lock object
  * itself, so actual lookup of the hash should be once per lock object.
  */
-static inline struct lock_class *
+static struct lock_class *
 register_lock_class(struct lockdep_map *lock, unsigned int subclass, int force)
 {
 	struct lockdep_subclass_key *key;

Re: [PATCH] drivers/infiniband/hw/nes/nes_verbs.c: Deinline nes_free_qp_mem, save 1072 bytes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

On Fri, Apr 08, 2016 at 08:58:42PM +0200, Denys Vlasenko wrote:
> This function compiles to 550 bytes of machine code.
> Three callsites, all in nes_create_qp.

I agree with you, the functions which calls below and after this
function are not optimized for speed and there is no need to inline
this function.

I have two requests from you.
1)
Can you please change title to be more convenient?
[PATCH] drivers/infiniband/hw/nes/nes_verbs.c: Deinline nes_free_qp_mem, save 1072 bytes
--->
[PATCH] IB/nes: Deinline nes_free_qp_mem

2) Add bloat-o-meter output to the commit message.

And after that feel free to add my RB tag.

Reviewed-By: Leon Romanovsky <leonro@mellanox.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]