Thanks a lot for the explanation and suggestion, Thomas, ;D
Please see my reply inline,
On Thu, Mar 13, 2014 at 5:53 AM, Thomas Graf <tg...@redhat.com> wrote:
> This looks great already. Some minor stuff.
>
>
> On 03/08/2014 03:04 AM, Alex Wang wrote:
>
>> @@ -1459,7 +1459,7 @@ static const struct nla_policy
>> vport_policy[OVS_VPORT_ATTR_MAX + 1] = {
>> [OVS_VPORT_ATTR_STATS] = { .len = sizeof(struct ovs_vport_stats)
>> },
>> [OVS_VPORT_ATTR_PORT_NO] = { .type = NLA_U32 },
>> [OVS_VPORT_ATTR_TYPE] = { .type = NLA_U32 },
>> - [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_U32 },
>> + [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_UNSPEC },
>> [OVS_VPORT_ATTR_OPTIONS] = { .type = NLA_NESTED },
>> };
>>
>
> I can understand that you want to change the policy to reflect
> the change in the extended format. Technically, staying at
> NLA_U32 would result in the same as you have now: enforce a
> minimal payload size of 4 bytes.
>
> This change might raise eyebrows though in the push to netdev
> as it looks like an ABI breakage without closer inspection.
>
>
Thx, I'll take your advice. And take the benefit of being clear + minimal
payload check.
> +int ovs_vport_set_upcall_portids(struct vport *vport, struct nlattr
>> *ids)
>> +{
>> + struct vport_portids *old, *vport_portids;
>> +
>> + if (!nla_len(ids) || nla_len(ids) % sizeof(u32))
>> + return -EINVAL;
>>
>
> This may crash if OVS_VPORT_ATTR_UPCALL_PID is not provided and ids
> becomes NULL since you removed the if (a[OVS_VPORT_ATTR_UPCALL_PID])
> in ovs_vport_cmd_set().
>
>
Yes, I'll add a check in ovs_vport_cmd_set().
> + struct vport_portids *ids;
>> + int err = 0;
>> +
>> + ids = rcu_dereference_ovsl(vport->upcall_portids);
>> +
>> + if (vport->dp->user_features & OVS_DP_F_VPORT_PIDS) {
>> + if (nla_put(skb, OVS_VPORT_ATTR_UPCALL_PID,
>> + ids->n_ids * sizeof(u32), (void *) ids->ids))
>> + err = -EMSGSIZE;
>> +
>> + } else {
>> + if (nla_put_u32(skb, OVS_VPORT_ATTR_UPCALL_PID,
>> + *ids->ids))
>> + err = -EMSGSIZE;
>> + }
>> +
>> + return err;
>>
>
> If you want to reduce code size here, you can simply return nla_put*()
> and get rid of the additional branching since these functions already
> return -EMSGSIZE.
thx for pointing this out, I'll modify accordingly.
>
> +u32 ovs_vport_find_portid(const struct vport *p, struct sk_buff *skb)
>> +{
>> + struct vport_portids *ids;
>> +
>> + ids = rcu_dereference_ovsl(p->upcall_portids);
>> +
>> + if (ids->n_ids == 1 && *ids->ids == 0)
>> + return 0;
>> +
>> + return ids->ids[skb_get_rxhash(skb) % ids->n_ids];
>>
>
> I have some dislike for the division here. I realize this is the
> slow path but no point in adding it if it can be avoided.
> Worst case, this is exploitable in a DoS scenario.
>
> One idea here might be to look at RPS and see how it avoided
> this using flow masks, see we().
>
Thx for helping me realize this issue~!
After asking around, there are two solutions listed as follows:
1. Ask user to specify the number of handlers to be power of 2. This way,
we can use the bitwise AND for modular operation.
2. Use the reciprocal division. At time of setting the 'upcall_portids',
we compute the reciprocal_value of the 'n_ids'.
And the ovs_vport_find_portid() is changed to:
"""
@@ -445,13 +437,14 @@ int ovs_vport_get_upcall_portids(const struct vport
*vport,
u32 ovs_vport_find_portid(const struct vport *p, struct sk_buff *skb)
{
struct vport_portids *ids;
+ u32 hash;
ids = rcu_dereference_ovsl(p->upcall_portids);
if (ids->n_ids == 1 && *ids->ids == 0)
return 0;
- return ids->ids[skb_get_rxhash(skb) % ids->n_ids];
+ hash = skb_get_rxhash(skb);
+ return ids->ids[hash - ids->n_ids * reciprocal_div(hash,
ids->rn_ids)];
}"""
We tend to take the second one, in that, we want to allow user to flexibly
configure the number of threads in userspace,
and the change of division to "one subtract, two multiplication and one
shift" saves the cpu cycles. (There may be better
way of using reciprocal_div() than the one I gave)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
