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 <b...@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 <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().
>
>
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;
> > }
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
