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

[Ryan Wilson]

Just one comment below. Otherwise, all other issues will be addressed in
the next patch.

Thanks!


On Tue, Jun 24, 2014 at 8:44 AM, Ben Pfaff <blp at nicira.com> wrote:

> 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().
>
>
Its marked this way because when creating a dp_netdev, I must initialize
and lock the dp->upcall_rwlock. If I don't add
OVS_NO_THREAD_SAFETY_ANALYSIS, sparse complains dp->upcall_rwlock is locked
at the end of the function. Unfortunately, I can't use
OVS_ACQUIRES(dp->upcall_rwlock) since create_dp_netdev() creates the
dp_netdev and does not take dp as an argument. Is there another way we deal
with issues like this?



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