[ovs-dev] [PATCH 3/5] datapath: Allow each vport to have an array of 'port_id's.
Alex Wang
alexw at nicira.com
Fri Mar 7 23:49:16 UTC 2014
Thanks for the review Thomas~ ;D
Please see my reply inline:
It's nice to see that you preserve backwards compatibility but why
> rename the attribute? It doesn't really serve a purpose except that you
> break the API.
>
> Since OVS_VPORT_ATTR_UPCALL_PID was a single u32 it is already forward
> compatible with the new array you introduce since Netlink on the kernel
> side only enforces a minimal and no maximum attribute payload size.
>
> OTOH, since the OVS user space peer enforces a maximum attribute length
> for NLA_U32 newer kernels will break older user space binaries. It can
> only happen if you downgrade while the DP remains loaded though.
Thanks for pointing this out. Still new to kernel programming... lot to
learn~
I'll keep the name OVS_VPORT_ATTR_UPCALL_PID. But, i'd still like to
change the
type to NLA_UNSPEC, since it can now be an array of netlink pids.
I check the code here:
http://www.cs.fsu.edu/~baker/devices/lxr/http/source/linux/lib/nlattr.c#L27
that the min_len for NLA_UNSPEC is 0. To avoid user passing in
less-than-four-byte
attribute, I'll add the following check:
@@ -376,7 +376,7 @@ int ovs_vport_set_upcall_portids(struct vport *vport,
struct nlattr *ids)
{
struct vport_portids *old, *vport_portids;
- if (nla_len(ids) % sizeof(u32))
+ if (!nla_len(ids) || nla_len(ids) % sizeof(u32))
return -EINVAL;
Also I will use the OVS_DP_ATTR_USER_FEATURES to allow datapath to have
knowledge of the userspace implementation. And will guarantee the
compatibility
in all directions.
> @@ -162,8 +164,13 @@ struct vport *ovs_vport_alloc(int priv_size, const
>> struct vport_ops *ops,
>> */
>> void ovs_vport_free(struct vport *vport)
>> {
>> + struct nlattr *a = kzalloc(sizeof *a + sizeof(u32), GFP_KERNEL);
>> +
>> + a->nla_len = sizeof(u32);
>> + ovs_vport_set_upcall_portids(vport, a);
>> free_percpu(vport->percpu_stats);
>> kfree(vport);
>> + kfree(a);
>> }
>>
>
> This is really ugly, any way to avoid this? :)
Yes, i missed the comment which says that the caller guarantees the elapse
of grace
period before calling this function. I'll use kfree(vport->upcall_portids)
directly.
> + * Must be called with rcu_read_lock.
>> + */
>> +int ovs_vport_set_upcall_portids(struct vport *vport, struct nlattr
>> *ids)
>> +{
>> + struct vport_portids *old, *vport_portids;
>> +
>> + if (nla_len(ids) % sizeof(u32))
>> + return -EINVAL;
>>
>
> n_ids must be validated to be > 0, otherwise division by 0, see later
> on.
The check I add above will guarantee n_ids > 0.
>
>
> + memcpy(vport_portids->ids, nla_data(ids), nla_len(ids));
>> +
>> + rcu_assign_pointer(vport->upcall_portids, vport_portids);
>> +
>> + if (old)
>> + call_rcu(&old->rcu, vport_portids_destroy_rcu_cb);
>>
>
> kfree_rcu()?
kfree_rcu() is still a relatively new function, we cannot use it for
compatibility reasons.
we will use it in upstream ovs module
> + ids = rcu_dereference_ovsl(vport->upcall_portids);
>> +
>> + if (nla_put(skb, OVS_VPORT_ATTR_UPCALL_PIDS,
>> + ids->n_ids * sizeof *ids->ids,
>>
>
> I would hardcode this as n_ids * sizeof(u32), Netlink ABI is set
> in stone.
>
>
Thanks, I'll do that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140307/d1f0a0ad/attachment-0005.html>
More information about the dev
mailing list