[ovs-dev] [PATCH V2] dpif: Change dpif API to allow multiple handler threads read upcall.

[Alex Wang]

Thanks Han,~

Your suggestion makes sense.  I'll add a "bool upcall_enable" in "struct
dpif_linux"
and detach the refresh_channels() from recv_set().

Alex Wang,


On Thu, Mar 6, 2014 at 5:38 AM, Han Zhou <zhouhan at gmail.com> wrote:

> Hi Alex,
>
> On Wed, 2014-03-05 at 16:34 -0800, Alex Wang wrote:
>
> > @@ -352,17 +358,45 @@ struct dpif_class {
> >      /* Enables or disables receiving packets with dpif_recv() for
> 'dpif'.
> >       * Turning packet receive off and then back on is allowed to change
> Netlink
> >       * PID assignments (see ->port_get_pid()).  The client is
> responsible for
> > -     * updating flows as necessary if it does this. */
> > -    int (*recv_set)(struct dpif *dpif, bool enable);
> > +     * updating flows as necessary if it does this.
> > +     *
> > +     * The 'n_handlers' specifies the number of upcall handlers there
> are.
> > +     * Since multiple upcall handlers can read upcalls simultaneously
> from
> > +     * 'dpif', each port can have multiple Netlink sockets, one per
> upcall
> > +     * handler.  And recv_set() is responsible for the following tasks:
> > +     *
> > +     *    When receiving is enabled:
> > +     *
> > +     *    - create 'n_handlers' Netlink sockets for each port.
> > +     *
> > +     *    - create 'n_handlers' poll loops, one for each upcall handler.
> > +     *
> > +     *    - registering the Netlink sockets for the same upcall handler
> to
> > +     *      the corresponding poll loop.
> > +     *
> > +     *    When receiving is disabled:
> > +     *
> > +     *    - the opposite of above.
> > +     *
> > +     * */
> > +    int (*recv_set)(struct dpif *dpif, bool enable, uint32_t
> n_handlers);
> > +
> > +    /* Refreshes the poll loops and Netlink sockets associated to each
> port,
> > +     * when the number of upcall handlers is changed to 'n_handlers' and
> > +     * receiving packets is enabled for 'dpif'. */
> > +    int (*handlers_set)(struct dpif *dpif, uint32_t n_handlers);
>
> Now since you already have a separate interface handlers_set() to update
> n_handlers, why still keep the n_handlers parameter in recv_set()? This
> makes the meaning of recv_set() unclear when it is called with "enabled"
> state equal to current recv state. See the implementation of
> dpif_linux_recv_set__() in dpif-linux.c in patch 4/5.
>
> In my opinion it would be more clear if:
> - handlers_set() is responsible for setting/updating handler number and
> refreshing channels if recv is already enabled.
> - recv_set() only enable/disables receiving. When enabling, use
> current/default n_handlers to create channels.
>
> Best regards,
> Han
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140306/ace109f5/attachment-0005.html>