On Thu, Aug 16, 2012 at 12:42:33PM -0400, Ed Maste wrote:
> On 16 August 2012 11:42, Ben Pfaff <b...@nicira.com> wrote:
> > On Thu, Aug 16, 2012 at 10:22:21AM -0400, Ed Maste wrote:
> >> On 15 August 2012 19:12, Ben Pfaff <b...@nicira.com> wrote:
> >> > This is easy enough, so it seems worthwhile now that FreeBSD is starting
> >> > to make more use of the "userspace switch".
> >> >
> >> > CC: Ed Maste <ema...@freebsd.org>
> >> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> >> > ---
> >> > lib/dpif-netdev.c | 33 +++++++++++++++++++--------------
> >> > 1 files changed, 19 insertions(+), 14 deletions(-)
> >> >
> >> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >> > index 3d01b17..144b6b6 100644
> >> > --- a/lib/dpif-netdev.c
> >> > +++ b/lib/dpif-netdev.c
> >> > @@ -70,8 +70,13 @@ enum { MAX_QUEUE_LEN = 128 }; /* Maximum number of
> >> > packets per queue. */
> >> > enum { QUEUE_MASK = MAX_QUEUE_LEN - 1 };
> >> > BUILD_ASSERT_DECL(IS_POW2(MAX_QUEUE_LEN));
> >> >
> >> > +struct dp_netdev_upcall {
> >> > + struct dpif_upcall upcall; /* Queued upcall information. */
> >> > + struct ofpbuf buf; /* ofpbuf instance for upcall.packet. */
> >> > +};
> >> > +
> >> > struct dp_netdev_queue {
> >> > - struct dpif_upcall *upcalls[MAX_QUEUE_LEN];
> >> > + struct dp_netdev_upcall upcalls[MAX_QUEUE_LEN];
> >> > unsigned int head, tail;
> >> > };
> >> >
> >> > @@ -259,10 +264,8 @@ dp_netdev_purge_queues(struct dp_netdev *dp)
> >> > struct dp_netdev_queue *q = &dp->queues[i];
> >> >
> >> > while (q->tail != q->head) {
> >> > - struct dpif_upcall *upcall = q->upcalls[q->tail++ &
> >> > QUEUE_MASK];
> >> > -
> >> > - ofpbuf_delete(upcall->packet);
> >> > - free(upcall);
> >> > + struct dp_netdev_upcall *u = &q->upcalls[q->tail++ &
> >> > QUEUE_MASK];
> >> > + ofpbuf_uninit(&u->buf);
> >> > }
> >> > }
> >> > }
> >> > @@ -960,13 +963,13 @@ dpif_netdev_recv(struct dpif *dpif, struct
> >> > dpif_upcall *upcall,
> >> > {
> >> > struct dp_netdev_queue *q = find_nonempty_queue(dpif);
> >> > if (q) {
> >> > - struct dpif_upcall *u = q->upcalls[q->tail++ & QUEUE_MASK];
> >> > - *upcall = *u;
> >> > - free(u);
> >> > + struct dp_netdev_upcall *u = &q->upcalls[q->tail++ &
> >> > QUEUE_MASK];
> >> > +
> >> > + *upcall = u->upcall;
> >> > + upcall->packet = buf;
> >>
> >> Doesn't upcall->key still point into the static buf from upcalls[] in
> >> this case?
> >
> > Yes. We've copied that buf into the function's argument and transferred
> > ownership of its data to the caller. The next time the static buf gets
> > used it will be reinitialized with a newly allocated data buffer.
>
> Ahh, of course. This looks good to me.
It's not your fault. This code is terribly, terribly confusing.
Thanks for the review, I pushed this to master.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
