On Wed, Jun 18, 2014 at 11:07:12AM -0700, Ryan Wilson wrote:
> Typically, kernel datapath threads send upcalls to userspace where
> handler threads process the upcalls. For TAP and DPDK devices, the
> datapath threads operate in userspace, so there is no need for
> separate handler threads.
>
> This patch allows userspace datapath threads to directly call the
> ofproto upcall functions, eliminating the need for handler threads
> for datapaths of type 'netdev'.
>
> Signed-off-by: Ryan Wilson <wr...@nicira.com>
This isn't a review, but I noticed a few stylistic points on a scan.
dp_netdev_disable_upcall() and dp_netdev_enable_upcall() are declared
"inline". It's better not to do that because it does not usually help
code generation and it does disable compiler warnings if a function is
unused.
I'm surprised that dpif_netdev_disable_upcall() is marked
OVS_NO_THREAD_SAFETY_ANALYSIS instead of
OVS_ACQUIRES(dp->upcall_rwlock). Similarly for
dpif_netdev_enable_upcall().
I found the comments on these functions kind of mystifying. Maybe there
needs to be a higher-level comment explaining what happens:
> + /* Registers an upcall callback function with 'dpif' if 'dpif' does not
> use
> + * handlers but instead directly executes upcall functions in ofproto. */
> + void (*register_upcall_cb)(struct dpif *, exec_upcall_cb *);
> +
> + /* Enables upcalls if 'dpif' directly executes upcall functions in
> + * ofproto. */
> + void (*enable_upcall)(struct dpif *);
> +
> + /* Enables upcalls if 'dpif' directly executes upcall functions in
> + * ofproto. */
> + void (*disable_upcall)(struct dpif *);
> };
The "{"s following the "if"s below should be on the same line as the
"if"s:
> +void
> +dpif_register_upcall_cb(struct dpif *dpif, exec_upcall_cb *cb)
> +{
> + if (dpif->dpif_class->register_upcall_cb)
> + {
> + dpif->dpif_class->register_upcall_cb(dpif, cb);
> + }
> +}
> +
> +void
> +dpif_enable_upcall(struct dpif *dpif)
> +{
> + if (dpif->dpif_class->enable_upcall)
> + {
> + dpif->dpif_class->enable_upcall(dpif);
> + }
> +}
> +
> +void
> +dpif_disable_upcall(struct dpif *dpif)
> +{
> + if (dpif->dpif_class->disable_upcall)
> + {
> + dpif->dpif_class->disable_upcall(dpif);
> + }
> +}
In dpif_recv() here the check for "error" in the second "if" statement
is unnecessary, because we already handled the !error case:
> + if (!error) {
> + dpif_print_packet(dpif, upcall);
> + } else if (error && error != EAGAIN) {
> + log_operation(dpif, "recv", error);
> + }
> }
> return error;
> }
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
