Re: [patch net-next 00/15] mlxsw: Reflect nexthop status changes

[David Miller]

From: Jiri Pirko 
Date: Wed, 8 Feb 2017 21:58:35 +0100

> Wed, Feb 08, 2017 at 09:43:45PM CET, da...@davemloft.net wrote:
>>From: David Miller 
>>Date: Wed, 08 Feb 2017 15:28:48 -0500 (EST)
>>
>>> Looks really nice, series applied, thanks!
>>
>>Jiri, just FYI, I bungled up merging this.  And I am trying to fix
>>that up.
>>
>>I forgot to include patch #14, but I'll apply that now in the
>>mainline.
> 
> Yeah, the lost patch #14. I had to resend it to get it to patchwork.
> That's probably what confused your tools.
> 
> 
>>
>>I get a warning, in mlxsw_sp_nexthop_group_create() because the
>>compiler thinks that 'nh' can be use uninitialized.  And I think
>>the compiler is pointing out something legitimate.
>>
>>This cleanup loop in err_nexthop_group_insert and err_nexthop_init
>>needs to take the 'nh' from _grp->nexthops[i] instead of using
>>whatever is left in 'nh' for all mlxsw_sp_nexthop_fini() calls.
>>
>>So I'm going to augment the commit of patch #14 to make it go:
>>
>>@@ -1667,8 +1667,10 @@ mlxsw_sp_nexthop_group_create(struct mlxsw_sp 
>>*mlxsw_sp, struct fib_info *fi)
>> 
>> err_nexthop_group_insert:
>> err_nexthop_init:
>>- for (i--; i >= 0; i--)
>>+ for (i--; i >= 0; i--) {
>>+ nh = _grp->nexthops[i];
>>  mlxsw_sp_nexthop_fini(mlxsw_sp, nh);
>>+ }
>>  kfree(nh_grp);
>>  return ERR_PTR(err);
>> }
> 
> Oh. I did not catch is. But this is totally unrelated to this patchset.
> Please send it as a separate fix. Or I can do it if yo want.
> 
> Fixes: a7ff87acd995 ("mlxsw: spectrum_router: Implement next-hop routing")
> Acked-by: Jiri Pirko 

Sorry, I integrated it into the patch #14 commit, and it's already pushed
out.

Re: [patch net-next 00/15] mlxsw: Reflect nexthop status changes

[David Miller]

From: Jiri Pirko 
Date: Wed,  8 Feb 2017 11:16:27 +0100

> From: Jiri Pirko 
> 
> Ido says:
> 
> When the kernel forwards IPv4 packets via multipath routes it doesn't
> consider nexthops that are dead or linkdown. For example, if the nexthop
> netdev is administratively down or doesn't have a carrier.
> 
> Devices capable of offloading such multipath routes need to be made
> aware of changes in the reflected nexthops' status. Otherwise, the
> device might forward packets via non-functional nexthops, resulting in
> packet loss. This patchset aims to fix that.
> 
> The first 11 patches deal with the necessary restructuring in the
> mlxsw driver, so that it's able to correctly add and remove nexthops
> from the device's adjacency table.
> 
> The 12th patch adds the NH_{ADD,DEL} events to the FIB notification
> chain. These notifications are sent whenever the kernel decides to add
> or remove a nexthop from the forwarding plane.
> 
> Finally, the last three patches add support for these events in the
> mlxsw driver, which is currently the only driver capable of offloading
> multipath routes.

Looks really nice, series applied, thanks!

Re: [patch net-next 00/15] mlxsw: Reflect nexthop status changes

[Ido Schimmel]

On Wed, Feb 08, 2017 at 03:43:45PM -0500, David Miller wrote:
> From: David Miller 
> Date: Wed, 08 Feb 2017 15:28:48 -0500 (EST)
> 
> > Looks really nice, series applied, thanks!
> 
> Jiri, just FYI, I bungled up merging this.  And I am trying to fix
> that up.
> 
> I forgot to include patch #14, but I'll apply that now in the
> mainline.
> 
> I get a warning, in mlxsw_sp_nexthop_group_create() because the
> compiler thinks that 'nh' can be use uninitialized.  And I think
> the compiler is pointing out something legitimate.
> 
> This cleanup loop in err_nexthop_group_insert and err_nexthop_init
> needs to take the 'nh' from _grp->nexthops[i] instead of using
> whatever is left in 'nh' for all mlxsw_sp_nexthop_fini() calls.
> 
> So I'm going to augment the commit of patch #14 to make it go:
> 
> @@ -1667,8 +1667,10 @@ mlxsw_sp_nexthop_group_create(struct mlxsw_sp 
> *mlxsw_sp, struct fib_info *fi)
>  
>  err_nexthop_group_insert:
>  err_nexthop_init:
> - for (i--; i >= 0; i--)
> + for (i--; i >= 0; i--) {
> + nh = _grp->nexthops[i];
>   mlxsw_sp_nexthop_fini(mlxsw_sp, nh);
> + }

Looks good to me. Thanks for fixing that up!

This is now consistent with mlxsw_sp_nexthop_group_destroy().


>   kfree(nh_grp);
>   return ERR_PTR(err);
>  }
> 

Re: [patch net-next 00/15] mlxsw: Reflect nexthop status changes

[Jiri Pirko]

Wed, Feb 08, 2017 at 09:43:45PM CET, da...@davemloft.net wrote:
>From: David Miller 
>Date: Wed, 08 Feb 2017 15:28:48 -0500 (EST)
>
>> Looks really nice, series applied, thanks!
>
>Jiri, just FYI, I bungled up merging this.  And I am trying to fix
>that up.
>
>I forgot to include patch #14, but I'll apply that now in the
>mainline.

Yeah, the lost patch #14. I had to resend it to get it to patchwork.
That's probably what confused your tools.


>
>I get a warning, in mlxsw_sp_nexthop_group_create() because the
>compiler thinks that 'nh' can be use uninitialized.  And I think
>the compiler is pointing out something legitimate.
>
>This cleanup loop in err_nexthop_group_insert and err_nexthop_init
>needs to take the 'nh' from _grp->nexthops[i] instead of using
>whatever is left in 'nh' for all mlxsw_sp_nexthop_fini() calls.
>
>So I'm going to augment the commit of patch #14 to make it go:
>
>@@ -1667,8 +1667,10 @@ mlxsw_sp_nexthop_group_create(struct mlxsw_sp 
>*mlxsw_sp, struct fib_info *fi)
> 
> err_nexthop_group_insert:
> err_nexthop_init:
>-  for (i--; i >= 0; i--)
>+  for (i--; i >= 0; i--) {
>+  nh = _grp->nexthops[i];
>   mlxsw_sp_nexthop_fini(mlxsw_sp, nh);
>+  }
>   kfree(nh_grp);
>   return ERR_PTR(err);
> }

Oh. I did not catch is. But this is totally unrelated to this patchset.
Please send it as a separate fix. Or I can do it if yo want.

Fixes: a7ff87acd995 ("mlxsw: spectrum_router: Implement next-hop routing")
Acked-by: Jiri Pirko 

Re: [patch net-next 00/15] mlxsw: Reflect nexthop status changes

[David Miller]

From: David Miller 
Date: Wed, 08 Feb 2017 15:28:48 -0500 (EST)

> Looks really nice, series applied, thanks!

Jiri, just FYI, I bungled up merging this.  And I am trying to fix
that up.

I forgot to include patch #14, but I'll apply that now in the
mainline.

I get a warning, in mlxsw_sp_nexthop_group_create() because the
compiler thinks that 'nh' can be use uninitialized.  And I think
the compiler is pointing out something legitimate.

This cleanup loop in err_nexthop_group_insert and err_nexthop_init
needs to take the 'nh' from _grp->nexthops[i] instead of using
whatever is left in 'nh' for all mlxsw_sp_nexthop_fini() calls.

So I'm going to augment the commit of patch #14 to make it go:

@@ -1667,8 +1667,10 @@ mlxsw_sp_nexthop_group_create(struct mlxsw_sp *mlxsw_sp, 
struct fib_info *fi)
 
 err_nexthop_group_insert:
 err_nexthop_init:
-   for (i--; i >= 0; i--)
+   for (i--; i >= 0; i--) {
+   nh = _grp->nexthops[i];
mlxsw_sp_nexthop_fini(mlxsw_sp, nh);
+   }
kfree(nh_grp);
return ERR_PTR(err);
 }

[patch net-next 00/15] mlxsw: Reflect nexthop status changes

[Jiri Pirko]

From: Jiri Pirko 

Ido says:

When the kernel forwards IPv4 packets via multipath routes it doesn't
consider nexthops that are dead or linkdown. For example, if the nexthop
netdev is administratively down or doesn't have a carrier.

Devices capable of offloading such multipath routes need to be made
aware of changes in the reflected nexthops' status. Otherwise, the
device might forward packets via non-functional nexthops, resulting in
packet loss. This patchset aims to fix that.

The first 11 patches deal with the necessary restructuring in the
mlxsw driver, so that it's able to correctly add and remove nexthops
from the device's adjacency table.

The 12th patch adds the NH_{ADD,DEL} events to the FIB notification
chain. These notifications are sent whenever the kernel decides to add
or remove a nexthop from the forwarding plane.

Finally, the last three patches add support for these events in the
mlxsw driver, which is currently the only driver capable of offloading
multipath routes.

Ido Schimmel (15):
  mlxsw: spectrum_router: Nullify nexthop's neigh pointer
  mlxsw: spectrum_router: Store nexthop groups in a hash table
  mlxsw: spectrum_router: Store nexthops in a hash table
  mlxsw: spectrum_router: Use nexthop's scope to set action type
  mlxsw: spectrum_router: Add gateway indication to nexthop group
  mlxsw: spectrum_router: Store routes in a more generic way
  mlxsw: spectrum_router: Remove FIB info from FIB entry struct
  mlxsw: spectrum_router: Refactor nexthop init routine
  mlxsw: spectrum_router: More accurately set offload flag
  mlxsw: spectrum_router: Determine offload status using generic
function
  mlxsw: spectrum_router: Use trap action only for some route types
  ipv4: fib: Notify about nexthop status changes
  mlxsw: spectrum_router: Reflect nexthop status changes
  mlxsw: spectrum_router: Don't reflect LINKDOWN nexthops
  mlxsw: spectrum_router: Flush resources when RIF is deleted

 drivers/net/ethernet/mellanox/mlxsw/spectrum.c |   6 +
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h |   7 +-
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 534 -
 include/net/ip_fib.h   |   7 +
 net/ipv4/fib_semantics.c   |  33 ++
 5 files changed, 457 insertions(+), 130 deletions(-)

-- 
2.7.4