netdev - Re: [RFC v1 for accelerated IPoIB 05/25] IB/ipoib: Support ipoib acceleration options callbacks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170313201049.GB2738@obsidianresearch.com>
Date:   Mon, 13 Mar 2017 14:10:49 -0600
From:   Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:     Erez Shitrit <erezsh@...lanox.com>
Cc:     dledford@...hat.com, linux-rdma@...r.kernel.org,
        netdev@...r.kernel.org, valex@...lanox.com, leonro@...lanox.com,
        saedm@...lanox.com, erezsh@....mellanox.co.il
Subject: Re: [RFC v1 for accelerated IPoIB 05/25] IB/ipoib: Support ipoib
 acceleration options callbacks

On Mon, Mar 13, 2017 at 08:31:16PM +0200, Erez Shitrit wrote:

> TODO: We added remote qkey to ipoib_send in order to match send op
> signature.
> In accel mode this param will be used but in regular mode this param is
> redundant. Need to think about better solution.

The flow is backwards, in accel mode the xmit ndo should be owend by
the driver and the driver should call a helper to get all the proper
AH data, including qkey.

> -static int ipoib_dev_init_default(struct net_device *dev, struct ib_device *ca,
> -				  int port)
> +static int ipoib_dev_init_default(struct net_device *dev,
> +				  struct ib_device *hca, int *qp_num)
>  {
> -	struct ipoib_dev_priv *priv = netdev_priv(dev);
> +	struct ipoib_dev_priv *priv = ipoib_priv(dev);
> +
> +	netif_napi_add(dev, &priv->napi, ipoib_poll, NAPI_POLL_WEIGHT);

All these 'default' functions are part of the 'rn driver'. They should
not be calling ipoib_priv, you said you didn't want ipoib_dev_priv
leaking into the drivers.

These _default funcs should use ipoib_dev_priv and all the members of
ipoib_dev_priv that are used exclusively by the 'default'
implementation need to be moved into a dedicated priv struct.

Otherwise the entire scheme become hugely confusing about what in
ipoib_dev_priv is actually valid in accel mode.

I think it would be much easier to maintain if the _default functions were
all in a dedicated files, eg rn_ipoib_ud_verbs.c

I also recommend splitting out the bulk rename of ipoib_priv into a
single patch with a '#define ipoib_priv(dev) netdev_priv(dev)'
shim. That would make this patch much smaller.

IHMO you probably don't need to send the mlx5 stuff until the series
up to here is OK. I think we all understand that mlx5 can implement
this API?

Jason