[ovs-dev] [PATCH v3] dpif-netdev: Polling threads directly call ofproto upcall functions.

Ben Pfaff blp at nicira.com
Tue Jun 24 15:44:52 UTC 2014


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 <wryan at 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;
>  }



More information about the dev mailing list