[ofa-general] Re: [PATCH] IB/ipoib: set neigh->dgid upon ipoib_neigh creation

[Yossi Etigin]

Hi Eli,

Wouldn't this patch break the fast bond+SM failover recovery? 

Up till now, in such case neigh->dgid was the old gid until path record 
query was successful, and that caused to retry the path query until it's 
successful. With that patch, a new path query will not be issued until 
the next arp refresh because neigh->dgid will be set to a valid value 
right after the first packet.
How about keeping the memcpy in path_rec-completion, and also adding it 
to ib_mcast_send (where ipoib neigh is created)?

--Yossi

Eli Cohen wrote:
> If we fail to do that, there can be superfluous calls to
> ipoib_neigh_free()/ipoib_neigh_alloc() due to the following if statement in
> ipoib_start_xmit():
> 
> 	if (unlikely((memcmp(&neigh->dgid.raw,
> 			     skb->dst->neighbour->ha + 4,
> 			     sizeof(union ib_gid))) ||
> 
> In the case of a unicast GID, it can happen until the path is resolved and is
> not so severe. In the case of a multicast address, neigh->dgid is never even
> updated, so the corresponding ipoib_neigh will be destroyed and created on
> every multicast packet sent.
> 
> Signed-off-by: Eli Cohen <eli at mellanox.co.il>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_main.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 0bd2a4f..0005107 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -460,8 +460,6 @@ static void path_rec_completion(int status,
>  			}
>  			kref_get(&path->ah->ref);
>  			neigh->ah = path->ah;
> -			memcpy(&neigh->dgid.raw, &path->pathrec.dgid.raw,
> -			       sizeof(union ib_gid));
>  
>  			if (ipoib_cm_enabled(dev, neigh->neighbour)) {
>  				if (!ipoib_cm_get(neigh))
> @@ -878,6 +876,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neighbour,
>  	neigh->neighbour = neighbour;
>  	neigh->dev = dev;
>  	*to_ipoib_neigh(neighbour) = neigh;
> +	memcpy(neigh->dgid.raw, neighbour->ha + 4, 16);
>  	skb_queue_head_init(&neigh->queue);
>  	ipoib_cm_set(neigh, NULL);
>