[ofa-general] Re: [RFC] [PATCH v3] IB/ipoib: Add bonding support to IPoIB

[Michael S. Tsirkin]

> Quoting Moni Shoua <monis at voltaire.com>:
> To fix it, this patch adds a dev field to struct ipoib_neigh which is used
> instead of the struct neighbour dev one.
> 
> Signed-off-by: Moni Shoua <monis at voltaire.com>
> Signed-off-by: Or Gerlitz <ogerlitz at voltaire.com>
> ---
> 
>  ipoib.h           |    4 +++-
>  ipoib_main.c      |   26 +++++++++++++-------------
>  ipoib_multicast.c |    2 +-
>  3 files changed, 17 insertions(+), 15 deletions(-)
> Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib.h
> ===================================================================
> --- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib.h	2007-01-25 11:05:32.000000000 +0200
> +++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib.h	2007-03-04 19:32:55.000000000 +0200
> @@ -216,6 +216,7 @@ struct ipoib_neigh {
>  	struct sk_buff_head queue;
>  
>  	struct neighbour   *neighbour;
> +	struct net_device *dev;
>  
>  	struct list_head    list;
>  };
> @@ -232,7 +233,8 @@ static inline struct ipoib_neigh **to_ip
>  				     INFINIBAND_ALEN, sizeof(void *));
>  }
>  
> -struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neigh);
> +struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neigh,
> +				      struct net_device *dev);
>  void ipoib_neigh_free(struct net_device *dev, struct ipoib_neigh *neigh);
>  
>  extern struct workqueue_struct *ipoib_workqueue;
> Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_main.c
> ===================================================================
> --- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib_main.c	2007-01-25 11:05:32.000000000 +0200
> +++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_main.c	2007-03-04 19:32:55.000000000 +0200
> @@ -248,7 +248,6 @@ static void path_free(struct net_device 
>  	struct ipoib_neigh *neigh, *tn;
>  	struct sk_buff *skb;
>  	unsigned long flags;
> -
>  	while ((skb = __skb_dequeue(&path->queue)))
>  		dev_kfree_skb_irq(skb);
>  
> @@ -490,7 +489,7 @@ static void neigh_add_path(struct sk_buf
>  	struct ipoib_path *path;
>  	struct ipoib_neigh *neigh;
>  
> -	neigh = ipoib_neigh_alloc(skb->dst->neighbour);
> +	neigh = ipoib_neigh_alloc(skb->dst->neighbour, skb->dev);
>  	if (!neigh) {
>  		++priv->stats.tx_dropped;
>  		dev_kfree_skb_any(skb);
> @@ -769,32 +768,32 @@ static void ipoib_set_mcast_list(struct 
>  static void ipoib_neigh_destructor(struct neighbour *n)
>  {
>  	struct ipoib_neigh *neigh;
> -	struct ipoib_dev_priv *priv = netdev_priv(n->dev);
> +	struct ipoib_dev_priv *priv;
>  	unsigned long flags;
>  	struct ipoib_ah *ah = NULL;
>  
> -	ipoib_dbg(priv,
> -		  "neigh_destructor for %06x " IPOIB_GID_FMT "\n",
> -		  IPOIB_QPN(n->ha),
> -		  IPOIB_GID_RAW_ARG(n->ha + 4));
> -
> -	spin_lock_irqsave(&priv->lock, flags);
>  
>  	neigh = *to_ipoib_neigh(n);
>  	if (neigh) {
> +		priv = netdev_priv(neigh->dev);
> +		ipoib_dbg(priv,
> +			  "neigh_destructor for %06x " IPOIB_GID_FMT "\n",
> +			  IPOIB_QPN(n->ha),
> +			  IPOIB_GID_RAW_ARG(n->ha + 4));
> +
> +		spin_lock_irqsave(&priv->lock, flags);
>  		if (neigh->ah)
>  			ah = neigh->ah;
>  		list_del(&neigh->list);
>  		ipoib_neigh_free(n->dev, neigh);
> +		spin_unlock_irqrestore(&priv->lock, flags);
>  	}
> -
> -	spin_unlock_irqrestore(&priv->lock, flags);
> -
>  	if (ah)
>  		ipoib_put_ah(ah);
>  }
>  
> -struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neighbour)
> +struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neighbour,
> +				      struct net_device *dev)
>  {
>  	struct ipoib_neigh *neigh;
>  
> @@ -803,6 +802,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(st
>  		return NULL;
>  
>  	neigh->neighbour = neighbour;
> +	neigh->dev = dev;
>  	*to_ipoib_neigh(neighbour) = neigh;
>  	skb_queue_head_init(&neigh->queue);
>  
> Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> ===================================================================
> --- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2007-01-25 11:05:32.000000000 +0200
> +++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2007-03-04 12:21:46.000000000 +0200
> @@ -774,7 +774,7 @@ out:
>  		if (skb->dst            &&
>  		    skb->dst->neighbour &&
>  		    !*to_ipoib_neigh(skb->dst->neighbour)) {
> -			struct ipoib_neigh *neigh = ipoib_neigh_alloc(skb->dst->neighbour);
> +			struct ipoib_neigh *neigh = ipoib_neigh_alloc(skb->dst->neighbour, skb->dev);
>  
>  			if (neigh) {
>  				kref_get(&mcast->ah->ref);

I'm re-reading this, and old archives.
Was the following problem addressed?

# It seems that in this design, if multiple ipoib interfaces are present, we might
# get an skb such that skb->dev will be different from the new dev field in struct
# ipoib_neigh.
# 
# ipoib_neigh ah field includes struct ib_ah *.
# This selects important parameters which depend on both packet source and
# destination interfaces.
# 
# It seems that the result will be that the packet will be sent on a wrong interface.
# Right?
# 
# I think the right thing might be to compare ipoib_neigh dev and
# skb->dev, and destroy ipoib_neigh if these do not match.
#
# However, this will affect performance negatively if this happens a lot.
# Need to understand the usage model for the bonding driver and whether
# there is some "locality" here.


-- 
MST