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 <zhou...@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
>
>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
