I'm happy with it, thanks.
Ethan
On Thu, Sep 12, 2013 at 5:42 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Thu, Sep 12, 2013 at 04:43:11PM -0700, Ben Pfaff wrote:
>> On Thu, Sep 12, 2013 at 04:39:01PM -0700, Ethan Jackson wrote:
>> > Well for the first suggestion, I think it simply comes down to doing a
>> > check if the reval_seq changed during handle_miss_upcalls() like we
>> > did before. If it changed then we skip the guarded_list_push_back().
>> > Of course there's a race window, but it's small and doesn't matter.
>>
>> I didn't realize you were OK with a small race. It's trivial then.
>> I'll change that.
>>
>> > For the second suggestion, I think we just need to change the infinite
>> > for loop to loop 50 times and give up if there's nothing there.
>>
>> No problem, will do.
>
> I folded this in, let me know if you want changes.
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 850633d..bc1e884 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -42,6 +42,7 @@ COVERAGE_DEFINE(upcall_queue_overflow);
> COVERAGE_DEFINE(drop_queue_overflow);
> COVERAGE_DEFINE(miss_queue_overflow);
> COVERAGE_DEFINE(fmb_queue_overflow);
> +COVERAGE_DEFINE(fmb_queue_revalidated);
>
> /* A thread that processes each upcall handed to it by the dispatcher thread,
> * forwards the upcall's packet, and then queues it to the main ofproto_dpif
> @@ -279,14 +280,16 @@ upcall_destroy(struct upcall *upcall)
> struct flow_miss_batch *
> flow_miss_batch_next(struct udpif *udpif)
> {
> - for (;;) {
> + int i;
> +
> + for (i = 0; i < 50; i++) {
> struct flow_miss_batch *next;
> unsigned int reval_seq;
> struct list *next_node;
>
> next_node = guarded_list_pop_front(&udpif->fmbs);
> if (!next_node) {
> - return NULL;
> + break;
> }
>
> next = CONTAINER_OF(next_node, struct flow_miss_batch, list_node);
> @@ -297,6 +300,8 @@ flow_miss_batch_next(struct udpif *udpif)
>
> flow_miss_batch_destroy(next);
> }
> +
> + return NULL;
> }
>
> /* Destroys and deallocates 'fmb'. */
> @@ -696,6 +701,7 @@ handle_miss_upcalls(struct udpif *udpif, struct list
> *upcalls)
> struct flow_miss_batch *fmb;
> size_t n_upcalls, n_ops, i;
> struct flow_miss *miss;
> + unsigned int reval_seq;
>
> /* Construct the to-do list.
> *
> @@ -792,11 +798,15 @@ handle_miss_upcalls(struct udpif *udpif, struct list
> *upcalls)
> }
> dpif_operate(udpif->dpif, opsp, n_ops);
>
> - if (guarded_list_push_back(&udpif->fmbs, &fmb->list_node,
> - MAX_QUEUE_LENGTH)) {
> - seq_change(udpif->wait_seq);
> - } else {
> + atomic_read(&udpif->reval_seq, &reval_seq);
> + if (reval_seq != fmb->reval_seq) {
> + COVERAGE_INC(fmb_queue_revalidated);
> + flow_miss_batch_destroy(fmb);
> + } else if (!guarded_list_push_back(&udpif->fmbs, &fmb->list_node,
> + MAX_QUEUE_LENGTH)) {
> COVERAGE_INC(fmb_queue_overflow);
> flow_miss_batch_destroy(fmb);
> + } else {
> + seq_change(udpif->wait_seq);
> }
> }
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
