Re: [PATCH V6 mlx5-next 11/16] RDMA/core: Add LAG functionality

[Maor Gottlieb <maorg@mellanox.com>]

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.