[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