On Tue, Apr 15, 2014 at 6:01 AM, Alex Wang <al...@nicira.com> wrote:
> Thx Pravin for the review, please see my comments inline,
>
>>
>> > + */
>> > +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;
>> > +
>> > + old = ovsl_dereference(vport->upcall_portids);
>> > +
>> > + vport_portids = kmalloc(sizeof *vport_portids + nla_len(ids),
>> > + GFP_KERNEL);
>> need to divide nla_len() by 4.
>
>
>
> Could you explain more a bit? Isn't nla_len returns the number of bytes in
> payload?
>
thats right. I miss read code.
>
>>
>> > + *
>> > + * Returns the portid of the target socket. Must be called with
>> > rcu_read_lock.
>> > + */
>> > +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);
>> > +
>> Is this function ever called under ovs-lock? if not we can just use
>> rcu_dereference.
>
>
>
> Thx, I'm clear about the usage of rcu_dereference_ovsl() and
> ovsl_dereference() now.
>
> Will fix it and the comments.
>
>
>
>>
>> > };
>> > +/**
>> > + * struct vport_portids - array of netlink portids of a vport.
>> > + * must be protected by rcu.
>> > + * @rcu: RCU callback head for deferred destruction.
>> > + * @n_ids: Size of @ids array.
>> > + * @rn_ids: The reciprocal value of @n_ids.
>> > + * @ids: Array storing the Netlink socket pids to use for packets
>> > received
>> > + * on this port that miss the flow table.
>> > + */
>> > +struct vport_portids {
>> > + u32 n_ids;
>> > + struct reciprocal_value rn_ids;
>> > + struct rcu_head rcu;
>> > + u32 *ids;
>> > +};
>> >
>> if we use u32 ids[], we can save pointer space. Same as we have done
>> in struct sw_flow{} for stats.
>
>
>
> Thx, I'll adjust accordingly,
>
>
>>
>> > /**
>> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> > index a88f6f1..b3977d1 100644
>> > --- a/include/linux/openvswitch.h
>> > +++ b/include/linux/openvswitch.h
>> > @@ -138,6 +138,9 @@ struct ovs_vport_stats {
>> > /* Allow last Netlink attribute to be unaligned */
>> > #define OVS_DP_F_UNALIGNED (1 << 0)
>> >
>> > +/* Allow datapath to associate multiple Netlink PIDs to each vport */
>> > +#define OVS_DP_F_VPORT_PIDS (1 << 1)
>> > +
>> Have you tested downgrade case with this patch?
>>
>
>
> Yes, I've tested it,
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
