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  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]
Date:   Wed, 29 Apr 2020 12:01:07 +0300
From:   Maor Gottlieb <maorg@...lanox.com>
To:     Jason Gunthorpe <jgg@...lanox.com>
Cc:     davem@...emloft.net, dledford@...hat.com, j.vosburgh@...il.com,
        vfalico@...il.com, andy@...yhouse.net, kuba@...nel.org,
        jiri@...lanox.com, dsahern@...nel.org, leonro@...lanox.com,
        saeedm@...lanox.com, linux-rdma@...r.kernel.org,
        netdev@...r.kernel.org, alexr@...lanox.com
Subject: Re: [PATCH V6 mlx5-next 11/16] RDMA/core: Add LAG functionality


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.

Powered by blists - more mailing lists