[ofa-general] Re: [RFC V3 PATCH 4/5] rdma/cma: implement RDMA_CM_EVENT_NETDEV_CHANGE notification

[Or Gerlitz]

Sean Hefty wrote:
>> +static void cma_ndev_work_handler(struct work_struct *_work)
>> +{
>> +	struct cma_ndev_work *work = container_of(_work, struct cma_ndev_work,
>> work);
>> +	struct rdma_id_private *id_priv = work->id;
>> +	int destroy = 0;
>> +
>> +	mutex_lock(&id_priv->handler_mutex);
>> +
>> +	if (id_priv->id.event_handler(&id_priv->id, &work->event)) {
> How do we know that the user hasn't tried to destroy the id from another
> callback?  We need some sort of state check here.
correct, will be fixed.

>> +		cma_exch(id_priv, CMA_DESTROYING);
>> +		destroy = 1;
>> +	}
>> +
>> +	cma_enable_remove(id_priv);
>
> I didn't see the matching cma_disable_remove() call.
As you can see also in the patch 3/5, places in the code which 
originally did --not-- call cma_enable_remove() but rather just did 
atomic_inc(&conn_id->dev_remove) were just replaced with 
mutex_lock(&id_priv->handler_mutex). This is b/c cma_enable_remove does 
two things: 1) it does the state validation 2) it locks the 
handler_mutex, so places in the code which don't need the state 
validation don't call it... a bit dirty.

>>
>> +static int cma_netdev_align_id(struct net_device *ndev, struct rdma_id_private
>> *id_priv)
>> +{
>
> nit - function name isn't clear to me.  Maybe something like
> cma_netdev_change_handler()?  Although I'm not sure that netdev change is what
> the user is really interested in.  What they really want to know is if IP
> address mapping/resolution changed.  netdev is hidden from the user.
OK, I will see how to improve the name
> Maybe call this RDMA_CM_EVENT_ADDR_CHANGE?
let me think about it
>> +static int cma_netdev_callback(struct notifier_block *self, unsigned long
>> event,
>> +	void *ctx)
>> +{
>> +	struct net_device *ndev = (struct net_device *)ctx;
>> +	struct cma_device *cma_dev;
>> +	struct rdma_id_private *id_priv;
>> +	int ret = NOTIFY_DONE;
>> +
>> +	if (dev_net(ndev) != &init_net)
>> +		return NOTIFY_DONE;
>> +
>> +	if (event != NETDEV_BONDING_FAILOVER)
>> +		return NOTIFY_DONE;
>> +
>> +	if (!(ndev->flags & IFF_MASTER) || !(ndev->priv_flags & IFF_BONDING))
>> +		return NOTIFY_DONE;
>> +
>> +	mutex_lock(&lock);
>> +	list_for_each_entry(cma_dev, &dev_list, list)
>
> It seems like we just need to find the cma_dev that has the current mapping
correct. So I can take the lock, find the device in the list, increment 
its refcount and release the lock.  For that end I would have to save in 
the cma device structure the names of the network devices which are 
associated with it.... i can't use a comarison of the pdev etc pointers 
to the dma device since some network devices (eg bonding / vlan 
interfaces in ethernet) have NULL pdev  (they are virtual devices)

Later I can scan this device ID list, but I must do it under the lock 
inorder not to race with the device removal code which removed IDs from 
this list in cma_process_remove(), correct?

Or.