[ovs-dev] [PATCH V3 1/5] dpif: Change dpif API to allow multiple handler threads read upcall.

[Ben Pfaff]

On Wed, Mar 12, 2014 at 09:31:31AM -0800, Alex Wang wrote:
> > - fold in Ben's clarification.
> > > - refine the comments.
> > > - invoke dpif_handlers_set() in udpif_set_threads().  this is a bug.
> > >   the previous code will cause the handlers polling from closed
> > >   fd.
> >
> > I'm still trying to figure out whether I properly understand the new
> > API.  Here's a suggested replacement for the first paragraph of the
> > comment on 'port_get_pid'.  Is it correct?
> >
> >     /* Returns the Netlink PID value to supply in OVS_ACTION_ATTR_USERSPACE
> >      * actions as the OVS_USERSPACE_ATTR_PID attribute's value, for use in
> >      * flows whose packets arrived on port 'port_no'.  In the case where
> > the
> >      * provider allocates multiple Netlink PIDs to a single port, it may
> > use
> >      * 'hash' to spread load among them.  The caller need not use a
> > particular
> >      * hash function, because it is not generally necessary to avoid
> > reordering
> >      * between packets received via flow misses (which are spread among
> > PIDs by
> >      * the datapath internally) and those received via userspace actions
> > (which
> >      * are spread via the return value of this function).  A 5-tuple hash
> > is
> >      * suitable.
> >
> >
> 
> Thanks for pointing out the reordering issue, the suggested comment makes
> sense.
> 
> I'm not sure if it is good grammar, but can I add one more 'between' like
> below?
> 
>     /* Returns the Netlink PID value to supply in OVS_ACTION_ATTR_USERSPACE
>      * actions as the OVS_USERSPACE_ATTR_PID attribute's value, for use in
>      * flows whose packets arrived on port 'port_no'.  In the case where the
>      * provider allocates multiple Netlink PIDs to a single port, it may use
>      * 'hash' to spread load among them.  The caller need not use a
> particular
>      * hash function, because it is not generally necessary to avoid
> reordering
>      * between packets received via flow misses (which are spread among
> PIDs by
>      * the datapath internally) and *between *those received via userspace
> actions (which
>      * are spread via the return value of this function), (e.g. sampling
> actions).
>      * A 5-tuple hash is suitable.

Hmm.  Usually one wants packets received via flow misses not to be
reordered.  Usually one wants packets received via userspace actions
not to be ordered.  One normally doesn't care about the relative
ordering of the packets received by the two means.  To me, adding
"between" in both cases means the former two statements, not the
latter.

How about something like this instead:

    Returns the Netlink PID value to supply in
    OVS_ACTION_ATTR_USERSPACE actions as the OVS_USERSPACE_ATTR_PID
    attribute's value, for use in flows whose packets arrived on port
    'port_no'.  In the case where the provider allocates multiple
    Netlink PIDs to a single port, it may use 'hash' to spread load
    among them.  The caller need not use a particular hash function; a
    5-tuple hash is suitable.

    (The datapath implementation might use some different hash
    function for distributing packets received via flow misses among
    PIDs.  This means that packets received via flow misses might be
    reordered relative to packets received via userspace actions.
    This is not ordinarily a problem.)