On 4/29/2020 2:30 AM, Jason Gunthorpe wrote:
On Tue, Apr 28, 2020 at 08:15:25PM -0300, Jason Gunthorpe wrote:
On Sun, Apr 26, 2020 at 10:17:12AM +0300, Maor Gottlieb wrote:
+int rdma_lag_get_ah_roce_slave(struct ib_device *device,
+ struct rdma_ah_attr *ah_attr,
+ struct net_device **xmit_slave)
Please do not use ** and also return int. The function should return
net_device directly and use ERR_PTR()
How about return NULL in failure as well (will add debug print)? Not
fail the flow if we didn't succeed to get the slave, let the lower
driver to do it if it would like to.
+{
+ struct net_device *master;
+ struct net_device *slave;
+ int err = 0;
+
+ *xmit_slave = NULL;
+ if (!(ah_attr->type == RDMA_AH_ATTR_TYPE_ROCE &&
+ ah_attr->grh.sgid_attr->gid_type == IB_GID_TYPE_ROCE_UDP_ENCAP))
+ return 0;
+
+ rcu_read_lock();
+ master = rdma_read_gid_attr_ndev_rcu(ah_attr->grh.sgid_attr);
+ if (IS_ERR(master)) {
+ err = PTR_ERR(master);
+ goto unlock;
+ }
+ dev_hold(master);
What is the point of this dev_hold? This whole thing is under
rcu_read_lock()
+
+ if (!netif_is_bond_master(master))
+ goto put;
+
+ slave = rdma_get_xmit_slave_udp(device, master, ah_attr);
IMHO it is probably better to keep with the dev_hold and drop the RCU
while doing rdma_build_skb so that the allocation in here doesn't have
to be atomic. This isn't performance sensitive so the extra atomic for
the dev_hold is better than the unnecessary GFP_ATOMIC allocation
Though if you do this be mindful that the create_ah call site is
conditionally non-sleeping, the best thing to do would be to make the
GFP_ATOMIC conditional on !RDMA_CREATE_AH_SLEEPABLE - ie pass in a gfp
flags argument.
Jason
Will go with your suggestion above.
