[ovs-dev] [PATCH V4 3/5] datapath: Allow each vport to have an array of 'port_id's.

Alex Wang alexw at nicira.com
Wed Mar 19 20:21:31 UTC 2014


Hey Thomas,

Thx a lot for the review, again, comments in line,

On Wed, Mar 19, 2014 at 4:37 AM, Thomas Graf <tgraf at redhat.com> wrote:

> On 03/18/2014 01:15 AM, Alex Wang wrote:
>
>> In order to allow handlers directly read upcalls from datapath,
>> we need to support per-handler netlink socket for each vport in
>> datapath.  This commit makes this happen.  Also, it is guaranteed
>> backward and forward compatibility with previous branch.
>>
>> Signed-off-by: Alex Wang <alexw at nicira.com>
>>
>
> FYI, recent netdev sub thread on the subject of % usage:
> http://www.spinics.net/lists/netdev/msg275467.html
>


Thanks for sharing the link.  I'll remember to take advantage of it, when
the divisor is constant.



> Patch looks good to me now.
>
> Acked-by: Thomas Graf <tgraf at redhat.com>
>
>
Great to see this!



> Two very minor comments included below just in case you need to
> respin anyway:
>
>  +int ovs_vport_set_upcall_portids(struct vport *vport,  struct nlattr
>> *ids)
>> +{
>> +       struct vport_portids *old, *vport_portids;
>> +
>>
> [...]
>
>  +       vport_portids->rn_ids = reciprocal_value(vport_portids->n_ids);
>> +       memcpy(vport_portids->ids, nla_data(ids), nla_len(ids));
>>
>
> I missed this before, this could be replaced with nla_memcpy().
>


Thanks, I'll take your suggestion,



>  +struct vport_portids {
>> +       struct rcu_head rcu;
>> +       u32 n_ids;
>> +       struct reciprocal_value rn_ids;
>> +       u32 *ids;
>> +};
>>
>
> rcu_head is typically placed at the end of structs for cachline
> efficiency as the struct grows (unlikely in this case ;)
>

Thx for brining up this issue.  I'll placed it on top of 'u32 *ids'.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140319/bd5ecf6a/attachment-0005.html>


More information about the dev mailing list