[ovs-dev] [PATCH V3] ofproto-dpif-upcall: Remove the dispatcher thread.

Alex Wang alexw at nicira.com
Thu Feb 27 19:43:13 UTC 2014


Thanks very much for the review!~ ;D

Please see my reply inline,


On Thu, Feb 20, 2014 at 3:25 PM, Pravin Shelar <pshelar at nicira.com> wrote:

> >                 upcall.cmd = OVS_PACKET_CMD_MISS;
>  >                 upcall.key = &key;
> >                 upcall.userdata = NULL;
> > -               upcall.portid = p->upcall_portid;
> > +               upcall.portid = ovs_vport_find_pid(p, &key);
> >                 ovs_dp_upcall(dp, skb, &upcall);
>
> Can you use different name than pid? pid is generally used for process id.
>
>


I'll use 'portid' for consistency.  In userspace, we call it pid (e.g.
nl_sock_pid(),
VPORT_ATTR_UPCALL_PID).  So I'll keep using it.





> > @@ -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 },
> >  };
> >
>
> Have you tried this patch with old userspace to check compatibility?



Will make sure the kernel module is compatible with old userspace.

Also, you just reminded me that this patch can be cut into series! ;D  So,
I'm
planning to do that and resend it as series of patches.




>  > @@ -1579,8 +1581,7 @@ static int ovs_vport_cmd_new(struct sk_buff *skb,
> struct genl_info *info)
> >         int err;
> >
> >         err = -EINVAL;
> > -       if (!a[OVS_VPORT_ATTR_NAME] || !a[OVS_VPORT_ATTR_TYPE] ||
> > -           !a[OVS_VPORT_ATTR_UPCALL_PID])
> > +       if (!a[OVS_VPORT_ATTR_NAME] || !a[OVS_VPORT_ATTR_TYPE])
> >                 goto exit;
> >
>
> Port ids array should be mandatory. as it is today.
>



Yeah, I'll change accordingly to honor the API and guarantee compatibility,





> > +static void __vport_pids_destroy(struct vport_pids *pids)
> > +{
> > +       if (pids->pids)
> > +               kfree(pids->pids);
> > +
> > +       kfree(pids);
> > +}
> > +
>
> This is not called from anywhere else. Can you use rcu-callback
> function to free?
>
> > +static void vport_pids_destroy_rcu_cb(struct rcu_head *rcu)
> > +{
> > +       struct vport_pids *pids = container_of(rcu, struct vport_pids,
> rcu);
> > +
> > +       __vport_pids_destroy(pids);
> > +}
>


I see, I'll combine the two.



> > + */
> > +int ovs_vport_set_upcall_pids(struct vport *vport,  struct nlattr *pids)
> > +{
> > +       struct vport_pids *old;
> > +
> > +       if (pids && nla_len(pids) % sizeof(u32))
> > +               return -EINVAL;
> > +
> > +       old = ovsl_dereference(vport->upcall_pids);
> > +
>
> +       if (pids) {
> > +               struct vport_pids *vport_pids;
> > +
> > +               vport_pids = kmalloc(sizeof *vport_pids, GFP_KERNEL);
> > +               vport_pids->pids = kmalloc(nla_len(pids), GFP_KERNEL);
> > +               vport_pids->n_pids = nla_len(pids)
> > +                       / (sizeof *vport_pids->pids);
> > +               memcpy(vport_pids->pids, nla_data(pids), nla_len(pids));
> > +
>
> We can just use single memory space to allocate both struct vport_pids
> and array. Same is done for flow-stats.
>
>


Thx for the good suggestion, I'll adjust my code accordingly.





>  > +               rcu_assign_pointer(vport->upcall_pids, vport_pids);
> > +       } else if (old) {
> > +               rcu_assign_pointer(vport->upcall_pids, NULL);
> > +       }
> > +
> > +       if (old)
> > +               call_rcu(&old->rcu, vport_pids_destroy_rcu_cb);
> > +
>
> This is bit confusing. We can free pids array in from vport destroy
> callback.
> So there is no  need to handle case where pids is NULL.
>
>


Sure, changes are made so  that vport_pids will never be NULL, except for
the first
time it is configured.




> > +int ovs_vport_get_upcall_pids(const struct vport *vport, struct sk_buff
> *skb)
> > +{
> > +       struct vport_pids *pids;
> > +       int err = 0;
> > +
> > +       rcu_read_lock();
> > +       pids = ovsl_dereference(vport->upcall_pids);
> > +
> This function is always called with rcu or ovs mutex. so there is no
> need to take extra lock.
>
>

I should check for that next time.  Fixed.



>  > +       if (!pids)
> > +               goto exit;
> > +
>
> pids should not be NULL in this case.



Yes, I'll remove it.



>  > + * Returns the pid of the target socket.  Must be called with
> rcu_read_lock.
> > + */
> > +u32 ovs_vport_find_pid(const struct vport *p, const struct sw_flow_key
> *key)
> > +{
> > +       struct vport_pids *pids;
> > +       u32 hash;
> > +
> > +       pids = ovsl_dereference(p->upcall_pids);
> > +
> This function is only called under rcu read context, therefore
> rcu_dereference is sufficient.
>
>

Thanks, fixed.




>  > +       if (!pids)
> > +               return 0;
> > +
> pids should not be NULL in this case.
>
>

Yes, I'll remove it.



>  > +       hash = key->ipv4.addr.src ^ key->ipv4.addr.dst
> > +               ^ key->ip.proto ^ key->ipv4.tp.src
> > +               ^ key->ipv4.tp.dst;
> > +
>
> skb_get_rxhash() should be cheaper in general than calculating hash.
>
>

Thanks for the suggestion, I'll use it.



>  > +       return pids->pids[jhash((void *) &hash, 4, 0) % pids->n_pids];
> > +}
> > +
>
>  We should use hash seed here since we are using values from packet
> coming from network.
>
>

Since i'll use skb_get_rxhash(), I will just used the skb_get_rxhash()
return value.
So, no seed is needed.




> > -        int idx = dpif->epoll_events[dpif->event_offset].data.u32;
> > -        struct dpif_channel *ch = &dpif->channels[idx];
> > +    while (epoll->event_offset < epoll->n_events) {
> > +        int idx = epoll->epoll_events[epoll->event_offset].data.u32;
> > +        struct dpif_channel *ch = &dpif->channels[idx][handler_id];
> >
>
> I have not looked at whole userspace part, but just noticed that this
> channels access pattern is not good for cpu cacheline.
> Is there reason for doing it this way rather than channel[handler_id][idx]
> ?
>


I just haven't thought about the cacheline benefits.  Will make the change.
 Thanks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140227/6150d5e8/attachment-0005.html>


More information about the dev mailing list