[ovs-dev] [PATCH 4/4] datapath: Hold rcu_read_lock where we claim to.

[Ben Pfaff]

On Wed, May 12, 2010 at 02:33:31PM -0700, Jesse Gross wrote:
> Many of the vport operations require that either RTNL lock or
> rcu_read_lock be held.  However, operations from userspace often
> hold a different lock so grab rcu_read_lock as well.

Comments below.

> diff --git a/datapath/vport.c b/datapath/vport.c
> index 0db9642..4ebb651 100644
> --- a/datapath/vport.c
> +++ b/datapath/vport.c
> @@ -350,9 +350,13 @@ vport_stats_get(struct odp_vport_stats_req __user *ustats_req)
>  		goto out;
>  	}
>  
> -	if (vport->ops->get_stats)
> +	if (vport->ops->get_stats) {
> +
> +		rcu_read_lock();
>  		err = vport->ops->get_stats(vport, &stats_req.stats);
> -	else if (vport->ops->flags & VPORT_F_GEN_STATS) {
> +		rcu_read_unlock();
> +

The extra blank lines around lock and unlock here look funny to me.

> +	}else if (vport->ops->flags & VPORT_F_GEN_STATS) {

I'd put a space before "else".

> @@ -582,10 +588,14 @@ vport_locate(const char *name)
>  		dump_stack();
>  	}
>  
> +	rcu_read_lock();
> +
>  	hlist_for_each_entry(vport, node, bucket, hash_node)
>  		if (!strcmp(name, vport_get_name(vport)))
>  			return vport;
>  
> +	rcu_read_unlock();
> +
>  	return NULL;
>  }

Is this really correct?  The callers don't expect vport_locate() to have
acquired the rcu_read_lock() for it on success.  And if there's an
omitted rcu_read_unlock() in the "if" clause then the RCU read lock
doesn't protect the caller.