[ovs-dev] [PATCH 1/6] datapath: Return vport configuration when queried.

Justin Pettit jpettit at nicira.com
Thu Dec 23 01:32:47 UTC 2010


On Dec 22, 2010, at 3:27 PM, Jesse Gross wrote:

> I don't think this patch is trivial enough to skip having a proper
> commit message.  In particular, I would expect it to explain *why* the
> change is being made.

Okay.  Done.

> In other places we use actually spell this out as 'port_config' and in
> general have avoided using initialisms like this as they are hard to
> read.

Okey-dokey.  Updated.

>> +       tpc = &rcu_dereference_rtnl(tnl_vport->mutable)->port_config;
>> +       memcpy(config, tpc, sizeof *tpc);
> 
> Kernel style is to use parenthesis with sizeof.

Okay.  Changed.  (There are quite a few other places in the datapath that don't follow that convention.)

>> +static void patch_get_config(const struct vport *vport, void *config)
>> +{
>> +       const struct patch_vport *patch_vport = patch_vport_priv(vport);
>> +       strlcpy(config, patch_vport->peer_name, VPORT_CONFIG_SIZE);
>> +}
> 
> This isn't SMP safe.  Previously, peer_name was only accessed from
> places where the port config was being changed and in those places
> RTNL mutex is always held.  However, when this is called we don't hold
> RTNL, so it is possible for peer_name to change during the copy.  The
> right way to do it is to move peer_name into struct device_config and
> use RCU to access it.

I've modified the patch to do that.  I'll send it out shortly.

>>  /**
>> + *     vport_get_config - retrieve device configuration
>> + *
>> + * @vport: vport from which to retrieve the configuration.
>> + * @config: buffer to store config, which must be at least the length
>> + *          of VPORT_CONFIG_SIZE.
>> + *
>> + * Retrieves the configuration of the given device.  Either RTNL lock or
>> + * rcu_read_lock must be held for the entire duration that the
>> + * configuration is in use.
> 
> This comment isn't accurate: since the caller provides the buffer,
> there's no need hold locks after the function call.  The need to
> continue holding the lock only applies to cases where a pointer into
> an internal data structure is returned.


Updated.

Thanks for the review.

-Justin






More information about the dev mailing list