[openib-general] tavor quirks etc

Or Gerlitz ogerlitz at voltaire.com
Tue Dec 19 04:37:31 PST 2006


Basically, i think we should be going to the simple approach of having 
**one** quirk in the rdma cm kernel code saying:

	if (tavor_quirk)
		then route->path_rec->mtu = IB_MTU_1024

so users would have to set the quirk to true in the presence of tavor
HCA either in the active or passive side.

This patch should also go upstream.

The problems i see with the current approach are:

1) there are three patches

2) of them, the cma-tavor-quirk is broken (see *** below) in its design
since it assumes the opensm-tavor-quirk and it would not work with 
opensm that does not have it nor with 3rd party/commercial SMs which do 
not have similar quirk

3) the ipoib-selector patch (below) in a way assumes the open-sm quirk
and hence it was not pushed upstream, and vise-versa an upstream ipoib
code is broken with the open-sm running with the quirk!

(***) per 15.2.5.16 PATHRECORD, you should get from the SM "less
than MTU specified" in case it has such path.

Now, what does it means that "it has such path"??? looking in the opensm 
  code @ opensm/osm_sa_path_record.c :: __osm_pr_rcv_get_path_parms

you can see that when the tavor quirk patch is ***not*** applied the sm 
scans the path and for each port compares the port mtu to the requested 
mtu, such that at the end of the scan the path mtu is the minimal mtu 
reported along the path. and then apply this code:

> if ( ( comp_mask & IB_PR_COMPMASK_MTUSELEC ) &&
>        ( comp_mask & IB_PR_COMPMASK_MTU ) )
>   {
>     required_mtu = ib_path_rec_mtu( p_pr );
>     switch( ib_path_rec_mtu_sel( p_pr ) )
>     {
>     case 0:    /* must be greater than */
>       if( mtu <= required_mtu )
>         status = IB_NOT_FOUND;
>       break;
> 
>     case 1:    /* must be less than */
>       if( mtu >= required_mtu )
>         status = IB_NOT_FOUND;
>       break;

XXX - the cma_tavor_quirk is broken without the opensm-tavor-quirk

> 
>     case 2:    /* exact match */
>       if( mtu != required_mtu )
>         status = IB_NOT_FOUND;
>       break;
> 
>     case 3:    /* largest available */
>       /* can't be disqualified by this one */
>       break;

this is the ipoib-selector patch

> Index: ofed_1_1/drivers/infiniband/ulp/ipoib/ipoib_main.c
> ===================================================================
> --- ofed_1_1.orig/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ ofed_1_1/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -182,6 +182,8 @@ static int ipoib_change_mtu(struct net_d
> 
>         dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
> 
> +       queue_work(ipoib_workqueue, &priv->flush_task);
> +
>         return 0;
>  }
> 
> @@ -452,15 +454,39 @@ static int path_rec_start(struct net_dev
>                           struct ipoib_path *path)
>  {
>         struct ipoib_dev_priv *priv = netdev_priv(dev);
> +       ib_sa_comp_mask comp_mask = IB_SA_PATH_REC_MTU_SELECTOR | IB_SA_PATH_REC_MTU;
> +
> +       path->pathrec.mtu_selector = IB_SA_GT;
> 
> -       ipoib_dbg(priv, "Start path record lookup for " IPOIB_GID_FMT "\n",
> -                 IPOIB_GID_ARG(path->pathrec.dgid));
> +       switch (roundup_pow_of_two(dev->mtu + IPOIB_ENCAP_LEN)) {
> +       case 512:
> +               path->pathrec.mtu = IB_MTU_256;
> +               break;
> +       case 1024:
> +               path->pathrec.mtu = IB_MTU_512;
> +               break;
> +       case 2048:
> +               path->pathrec.mtu = IB_MTU_1024;
> +               break;
> +       case 4096:
> +               path->pathrec.mtu = IB_MTU_2048;
> +               break;
> +       default:
> +               /* Wildcard everything */
> +               comp_mask = 0;
> +               path->pathrec.mtu = 0;
> +               path->pathrec.mtu_selector = 0;
> +       }
> +       ipoib_dbg(priv, "Start path record lookup for " IPOIB_GID_FMT " MTU > %d\n",
> +                 IPOIB_GID_ARG(path->pathrec.dgid),
> +                 comp_mask ? ib_mtu_enum_to_int(path->pathrec.mtu) : 0);
> 
>         init_completion(&path->done);
> 
>         path->query_id =
>                 ib_sa_path_rec_get(priv->ca, priv->port,
> -                                  &path->pathrec,
> +                                  &path->pathrec, comp_mask    |
>                                    IB_SA_PATH_REC_DGID          |
>                                    IB_SA_PATH_REC_SGID          |
>                                    IB_SA_PATH_REC_NUMB_PATH     |








More information about the general mailing list