[PATCH] mlx5: Remove call to ida_pre_get

[PATCH] mlx5: Remove call to ida_pre_get

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

The mlx5 driver calls ida_pre_get() in a loop for no readily apparent
reason.  The driver uses ida_simple_get() which will call ida_pre_get()
by itself and there's no need to use ida_pre_get() unless using
ida_get_new().

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
index 10e16381f20a..3ba07c7096ef 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
@@ -1647,7 +1647,6 @@ try_add_to_existing_fg(struct mlx5_flow_table *ft,
 
 	list_for_each_entry(iter, match_head, list) {
 		nested_down_read_ref_node(&iter->g->node, FS_LOCK_PARENT);
-		ida_pre_get(&iter->g->fte_allocator, GFP_KERNEL);
 	}
 
 search_again_locked:

Re: [PATCH] mlx5: Remove call to ida_pre_get

[Saeed Mahameed]

On Wed, 2018-03-14 at 19:57 -0700, Matthew Wilcox wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> The mlx5 driver calls ida_pre_get() in a loop for no readily apparent
> reason.  The driver uses ida_simple_get() which will call
> ida_pre_get()
> by itself and there's no need to use ida_pre_get() unless using
> ida_get_new().
> 

Hi Matthew,

Is this is causing any issues ? or just a simple cleanup ?

Adding Maor, the author of this change,

I believe the idea is to speed up insert_fte (which calls
ida_simple_get) since insert_fte runs under the FTE write semaphore,
in this case if ida_pre_get was successful before taking the semaphore
for all the FTE nodes in the loop, this will be a huge win for
ida_simple_get which will immediately return success without even
trying to allocate.

so it is a best effort to speed up critical path.

Maor, if this is really the case and this is not causing any issues,
then we need to consider adding a comment.


> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> index 10e16381f20a..3ba07c7096ef 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> @@ -1647,7 +1647,6 @@ try_add_to_existing_fg(struct mlx5_flow_table
> *ft,
>  
>  	list_for_each_entry(iter, match_head, list) {
>  		nested_down_read_ref_node(&iter->g->node,
> FS_LOCK_PARENT);
> -		ida_pre_get(&iter->g->fte_allocator, GFP_KERNEL);
>  	}
>  
>  search_again_locked:
> 

Re: [PATCH] mlx5: Remove call to ida_pre_get

[Matthew Wilcox]

On Thu, Mar 15, 2018 at 11:58:07PM +0000, Saeed Mahameed wrote:
> On Wed, 2018-03-14 at 19:57 -0700, Matthew Wilcox wrote:
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > The mlx5 driver calls ida_pre_get() in a loop for no readily apparent
> > reason.  The driver uses ida_simple_get() which will call
> > ida_pre_get()
> > by itself and there's no need to use ida_pre_get() unless using
> > ida_get_new().
> > 
> 
> Hi Matthew,
> 
> Is this is causing any issues ? or just a simple cleanup ?

I'm removing the API.  At the end of this cleanup, there will be no more
preallocation; instead we will rely on the slab allocator not sucking.

> Adding Maor, the author of this change,
> 
> I believe the idea is to speed up insert_fte (which calls
> ida_simple_get) since insert_fte runs under the FTE write semaphore,
> in this case if ida_pre_get was successful before taking the semaphore
> for all the FTE nodes in the loop, this will be a huge win for
> ida_simple_get which will immediately return success without even
> trying to allocate.

I think that's misguided.  The IDA allocator is only going to allocate
memory once in every 1024 allocations.  Also, it does try to allocate,
even if there are preallocated nodes.  So you're just wasting time,
unfortunately.

Re: [PATCH] mlx5: Remove call to ida_pre_get

[Saeed Mahameed]

On Thu, 2018-03-15 at 18:30 -0700, Matthew Wilcox wrote:
> On Thu, Mar 15, 2018 at 11:58:07PM +0000, Saeed Mahameed wrote:
> > On Wed, 2018-03-14 at 19:57 -0700, Matthew Wilcox wrote:
> > > From: Matthew Wilcox <mawilcox@microsoft.com>
> > > 
> > > The mlx5 driver calls ida_pre_get() in a loop for no readily
> > > apparent
> > > reason.  The driver uses ida_simple_get() which will call
> > > ida_pre_get()
> > > by itself and there's no need to use ida_pre_get() unless using
> > > ida_get_new().
> > > 
> > 
> > Hi Matthew,
> > 
> > Is this is causing any issues ? or just a simple cleanup ?
> 
> I'm removing the API.  At the end of this cleanup, there will be no
> more
> preallocation; instead we will rely on the slab allocator not
> sucking.
> 

Ok, Seems reasonable, I am ok with this.

> > Adding Maor, the author of this change,
> > 
> > I believe the idea is to speed up insert_fte (which calls
> > ida_simple_get) since insert_fte runs under the FTE write
> > semaphore,
> > in this case if ida_pre_get was successful before taking the
> > semaphore
> > for all the FTE nodes in the loop, this will be a huge win for
> > ida_simple_get which will immediately return success without even
> > trying to allocate.
> 
> I think that's misguided.  The IDA allocator is only going to
> allocate
> memory once in every 1024 allocations.  Also, it does try to
> allocate,
> even if there are preallocated nodes.  So you're just wasting time,
> unfortunately.
> 

Well just by looking at the code you can tell for sure that 
two consecutive calls to ida_pre_get will result in one allocation
only.
due to "if (!this_cpu_read(ida_bitmap))"

but i didn't dig into details and didn't go through the whole
ida_get_new_above, so i will count on your judgment here.

Still i would like to wait for Maor's input here, the author..
I Will ping him today.

Thanks,
Saeed.

Re: [PATCH] mlx5: Remove call to ida_pre_get

[Maor Gottlieb]

On 3/20/2018 5:29 AM, Saeed Mahameed wrote:
> On Thu, 2018-03-15 at 18:30 -0700, Matthew Wilcox wrote:
>> On Thu, Mar 15, 2018 at 11:58:07PM +0000, Saeed Mahameed wrote:
>>> On Wed, 2018-03-14 at 19:57 -0700, Matthew Wilcox wrote:
>>>> From: Matthew Wilcox <mawilcox@microsoft.com>
>>>>
>>>> The mlx5 driver calls ida_pre_get() in a loop for no readily
>>>> apparent
>>>> reason.  The driver uses ida_simple_get() which will call
>>>> ida_pre_get()
>>>> by itself and there's no need to use ida_pre_get() unless using
>>>> ida_get_new().
>>>>
>>> Hi Matthew,
>>>
>>> Is this is causing any issues ? or just a simple cleanup ?
>> I'm removing the API.  At the end of this cleanup, there will be no
>> more
>> preallocation; instead we will rely on the slab allocator not
>> sucking.
>>
> Ok, Seems reasonable, I am ok with this.
>
>>> Adding Maor, the author of this change,
>>>
>>> I believe the idea is to speed up insert_fte (which calls
>>> ida_simple_get) since insert_fte runs under the FTE write
>>> semaphore,
>>> in this case if ida_pre_get was successful before taking the
>>> semaphore
>>> for all the FTE nodes in the loop, this will be a huge win for
>>> ida_simple_get which will immediately return success without even
>>> trying to allocate.
>> I think that's misguided.  The IDA allocator is only going to
>> allocate
>> memory once in every 1024 allocations.  Also, it does try to
>> allocate,
>> even if there are preallocated nodes.  So you're just wasting time,
>> unfortunately.
>>
> Well just by looking at the code you can tell for sure that
> two consecutive calls to ida_pre_get will result in one allocation
> only.
> due to "if (!this_cpu_read(ida_bitmap))"
>
> but i didn't dig into details and didn't go through the whole
> ida_get_new_above, so i will count on your judgment here.
>
> Still i would like to wait for Maor's input here, the author..
> I Will ping him today.
>
> Thanks,
> Saeed.

Saeed, Matan and I okay with this fix as well, it looks like it 
shouldn't impact on the insertion rate.

Re: [PATCH] mlx5: Remove call to ida_pre_get

[David Miller]

From: Maor Gottlieb <maorg@mellanox.com>
Date: Tue, 20 Mar 2018 14:41:49 +0200

> Saeed, Matan and I okay with this fix as well, it looks like it
> shouldn't impact on the insertion rate.

I've applied this to net-next, thanks everyone.

Re: [PATCH] mlx5: Remove call to ida_pre_get

[Matthew Wilcox]

On Tue, Mar 20, 2018 at 10:46:20AM -0400, David Miller wrote:
> From: Maor Gottlieb <maorg@mellanox.com>
> Date: Tue, 20 Mar 2018 14:41:49 +0200
> 
> > Saeed, Matan and I okay with this fix as well, it looks like it
> > shouldn't impact on the insertion rate.
> 
> I've applied this to net-next, thanks everyone.

Thanks, Dave.

I realised why this made sense when it was originally written.  Before
December 2016 (commit 7ad3d4d85c7a), ida_pre_get used to allocate one
bitmap per ida.  I moved it to a percpu variable, and at that point this
stopped making sense.