How do you suggest changing the guarded-list abstraction to accommodate that?
On Thu, Sep 12, 2013 at 04:26:43PM -0700, Ethan Jackson wrote: > I'm not sure how much this matters in practice, but in > handle_miss_upcalls() I'd prefer we don't queue up fmbs which we know > are just going to get dropped later. I'm worried that the threads are > going to saturate the queue much more quickly than the main thread can > handle and we're going to have problems. > > Similarly I'd like flow_miss_batch_next() to be bounded. One could > imagine child threads pounding the main threads with invalid flows and > causing an infinite loop. Again, not sure how much it matters in > practice. > > Ethan > > On Thu, Sep 12, 2013 at 12:39 AM, Ben Pfaff <b...@nicira.com> wrote: > > We already had queues that were suitable for replacement by this data > > structure, and I intend to add another one later on. > > > > flow_miss_batch_ofproto_destroyed() did not work well with the guarded-list > > structure (it required either adding a lot more functions or breaking the > > abstraction) so I changed the caller to just use udpif_revalidate(). > > > > Checking reval_seq at the end of handle_miss_upcalls() also didn't work > > well with the abstraction, so I decided that since this was a corner case > > anyway it would be acceptable to just drop those in flow_miss_batch_next(). > > > > Signed-off-by: Ben Pfaff <b...@nicira.com> > > Acked-by: Ethan Jackson <et...@nicira.com> > > --- > > lib/automake.mk | 2 + > > lib/guarded-list.c | 97 ++++++++++++++++++++ > > lib/guarded-list.h | 41 +++++++++ > > ofproto/ofproto-dpif-upcall.c | 196 > > ++++++++++++----------------------------- > > ofproto/ofproto-dpif-upcall.h | 6 +- > > ofproto/ofproto-dpif.c | 77 ++++------------ > > 6 files changed, 216 insertions(+), 203 deletions(-) > > create mode 100644 lib/guarded-list.c > > create mode 100644 lib/guarded-list.h > > > > diff --git a/lib/automake.mk b/lib/automake.mk > > index da1896a..92cfc13 100644 > > --- a/lib/automake.mk > > +++ b/lib/automake.mk > > @@ -58,6 +58,8 @@ lib_libopenvswitch_a_SOURCES = \ > > lib/fatal-signal.h \ > > lib/flow.c \ > > lib/flow.h \ > > + lib/guarded-list.c \ > > + lib/guarded-list.h \ > > lib/hash.c \ > > lib/hash.h \ > > lib/hindex.c \ > > diff --git a/lib/guarded-list.c b/lib/guarded-list.c > > new file mode 100644 > > index 0000000..cbb2030 > > --- /dev/null > > +++ b/lib/guarded-list.c > > @@ -0,0 +1,97 @@ > > +/* > > + * Copyright (c) 2013 Nicira, Inc. > > + * > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > + * See the License for the specific language governing permissions and > > + * limitations under the License. > > + */ > > + > > +#include <config.h> > > + > > +#include "guarded-list.h" > > + > > +void > > +guarded_list_init(struct guarded_list *list) > > +{ > > + ovs_mutex_init(&list->mutex); > > + list_init(&list->list); > > + list->n = 0; > > +} > > + > > +void > > +guarded_list_destroy(struct guarded_list *list) > > +{ > > + ovs_mutex_destroy(&list->mutex); > > +} > > + > > +bool > > +guarded_list_is_empty(const struct guarded_list *list) > > +{ > > + bool empty; > > + > > + ovs_mutex_lock(&list->mutex); > > + empty = list->n == 0; > > + ovs_mutex_unlock(&list->mutex); > > + > > + return empty; > > +} > > + > > +/* If 'list' has fewer than 'max' elements, adds 'node' at the end of the > > list > > + * and returns the number of elements now on the list. > > + * > > + * If 'list' already has at least 'max' elements, returns 0 without > > modifying > > + * the list. */ > > +size_t > > +guarded_list_push_back(struct guarded_list *list, > > + struct list *node, size_t max) > > +{ > > + size_t retval = 0; > > + > > + ovs_mutex_lock(&list->mutex); > > + if (list->n < max) { > > + list_push_back(&list->list, node); > > + retval = ++list->n; > > + } > > + ovs_mutex_unlock(&list->mutex); > > + > > + return retval; > > +} > > + > > +struct list * > > +guarded_list_pop_front(struct guarded_list *list) > > +{ > > + struct list *node = NULL; > > + > > + ovs_mutex_lock(&list->mutex); > > + if (list->n) { > > + node = list_pop_front(&list->list); > > + list->n--; > > + } > > + ovs_mutex_unlock(&list->mutex); > > + > > + return node; > > +} > > + > > +size_t > > +guarded_list_pop_all(struct guarded_list *list, struct list *elements) > > +{ > > + size_t n; > > + > > + ovs_mutex_lock(&list->mutex); > > + list_move(elements, &list->list); > > + n = list->n; > > + > > + list_init(&list->list); > > + list->n = 0; > > + ovs_mutex_unlock(&list->mutex); > > + > > + return n; > > +} > > diff --git a/lib/guarded-list.h b/lib/guarded-list.h > > new file mode 100644 > > index 0000000..625865d > > --- /dev/null > > +++ b/lib/guarded-list.h > > @@ -0,0 +1,41 @@ > > +/* > > + * Copyright (c) 2013 Nicira, Inc. > > + * > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > + * See the License for the specific language governing permissions and > > + * limitations under the License. > > + */ > > + > > +#ifndef GUARDED_LIST_H > > +#define GUARDED_LIST_H 1 > > + > > +#include <stddef.h> > > +#include "compiler.h" > > +#include "list.h" > > +#include "ovs-thread.h" > > + > > +struct guarded_list { > > + struct ovs_mutex mutex; > > + struct list list; > > + size_t n; > > +}; > > + > > +void guarded_list_init(struct guarded_list *); > > +void guarded_list_destroy(struct guarded_list *); > > + > > +bool guarded_list_is_empty(const struct guarded_list *); > > + > > +size_t guarded_list_push_back(struct guarded_list *, struct list *, > > + size_t max); > > +struct list *guarded_list_pop_front(struct guarded_list *); > > +size_t guarded_list_pop_all(struct guarded_list *, struct list *); > > + > > +#endif /* guarded-list.h */ > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > > index ae856a4..850633d 100644 > > --- a/ofproto/ofproto-dpif-upcall.c > > +++ b/ofproto/ofproto-dpif-upcall.c > > @@ -23,6 +23,7 @@ > > #include "dynamic-string.h" > > #include "dpif.h" > > #include "fail-open.h" > > +#include "guarded-list.h" > > #include "latch.h" > > #include "seq.h" > > #include "list.h" > > @@ -79,26 +80,15 @@ struct udpif { > > struct handler *handlers; /* Miss handlers. */ > > size_t n_handlers; > > > > - /* Atomic queue of unprocessed drop keys. */ > > - struct ovs_mutex drop_key_mutex; > > - struct list drop_keys OVS_GUARDED; > > - size_t n_drop_keys OVS_GUARDED; > > - > > - /* Atomic queue of special upcalls for ofproto-dpif to process. */ > > - struct ovs_mutex upcall_mutex; > > - struct list upcalls OVS_GUARDED; > > - size_t n_upcalls OVS_GUARDED; > > - > > - /* Atomic queue of flow_miss_batches. */ > > - struct ovs_mutex fmb_mutex; > > - struct list fmbs OVS_GUARDED; > > - size_t n_fmbs OVS_GUARDED; > > + /* Queues to pass up to ofproto-dpif. */ > > + struct guarded_list drop_keys; /* "struct drop key"s. */ > > + struct guarded_list upcalls; /* "struct upcall"s. */ > > + struct guarded_list fmbs; /* "struct flow_miss_batch"es. */ > > > > /* Number of times udpif_revalidate() has been called. */ > > atomic_uint reval_seq; > > > > struct seq *wait_seq; > > - uint64_t last_seq; > > > > struct latch exit_latch; /* Tells child threads to exit. */ > > }; > > @@ -121,13 +111,10 @@ udpif_create(struct dpif_backer *backer, struct dpif > > *dpif) > > udpif->secret = random_uint32(); > > udpif->wait_seq = seq_create(); > > latch_init(&udpif->exit_latch); > > - list_init(&udpif->drop_keys); > > - list_init(&udpif->upcalls); > > - list_init(&udpif->fmbs); > > + guarded_list_init(&udpif->drop_keys); > > + guarded_list_init(&udpif->upcalls); > > + guarded_list_init(&udpif->fmbs); > > atomic_init(&udpif->reval_seq, 0); > > - ovs_mutex_init(&udpif->drop_key_mutex); > > - ovs_mutex_init(&udpif->upcall_mutex); > > - ovs_mutex_init(&udpif->fmb_mutex); > > > > return udpif; > > } > > @@ -153,9 +140,9 @@ udpif_destroy(struct udpif *udpif) > > flow_miss_batch_destroy(fmb); > > } > > > > - ovs_mutex_destroy(&udpif->drop_key_mutex); > > - ovs_mutex_destroy(&udpif->upcall_mutex); > > - ovs_mutex_destroy(&udpif->fmb_mutex); > > + guarded_list_destroy(&udpif->drop_keys); > > + guarded_list_destroy(&udpif->upcalls); > > + guarded_list_destroy(&udpif->fmbs); > > latch_destroy(&udpif->exit_latch); > > seq_destroy(udpif->wait_seq); > > free(udpif); > > @@ -229,33 +216,16 @@ udpif_recv_set(struct udpif *udpif, size_t > > n_handlers, bool enable) > > } > > > > void > > -udpif_run(struct udpif *udpif) > > -{ > > - udpif->last_seq = seq_read(udpif->wait_seq); > > -} > > - > > -void > > udpif_wait(struct udpif *udpif) > > { > > - ovs_mutex_lock(&udpif->drop_key_mutex); > > - if (udpif->n_drop_keys) { > > - poll_immediate_wake(); > > - } > > - ovs_mutex_unlock(&udpif->drop_key_mutex); > > - > > - ovs_mutex_lock(&udpif->upcall_mutex); > > - if (udpif->n_upcalls) { > > - poll_immediate_wake(); > > - } > > - ovs_mutex_unlock(&udpif->upcall_mutex); > > - > > - ovs_mutex_lock(&udpif->fmb_mutex); > > - if (udpif->n_fmbs) { > > + uint64_t seq = seq_read(udpif->wait_seq); > > + if (!guarded_list_is_empty(&udpif->drop_keys) || > > + !guarded_list_is_empty(&udpif->upcalls) || > > + !guarded_list_is_empty(&udpif->fmbs)) { > > poll_immediate_wake(); > > + } else { > > + seq_wait(udpif->wait_seq, seq); > > } > > - ovs_mutex_unlock(&udpif->fmb_mutex); > > - > > - seq_wait(udpif->wait_seq, udpif->last_seq); > > } > > > > /* Notifies 'udpif' that something changed which may render previous > > @@ -265,20 +235,21 @@ udpif_revalidate(struct udpif *udpif) > > { > > struct flow_miss_batch *fmb, *next_fmb; > > unsigned int junk; > > + struct list fmbs; > > > > /* Since we remove each miss on revalidation, their statistics won't be > > * accounted to the appropriate 'facet's in the upper layer. In most > > * cases, this is alright because we've already pushed the stats to the > > * relevant rules. However, NetFlow requires absolute packet counts on > > * 'facet's which could now be incorrect. */ > > - ovs_mutex_lock(&udpif->fmb_mutex); > > atomic_add(&udpif->reval_seq, 1, &junk); > > - LIST_FOR_EACH_SAFE (fmb, next_fmb, list_node, &udpif->fmbs) { > > + > > + guarded_list_pop_all(&udpif->fmbs, &fmbs); > > + LIST_FOR_EACH_SAFE (fmb, next_fmb, list_node, &fmbs) { > > list_remove(&fmb->list_node); > > flow_miss_batch_destroy(fmb); > > - udpif->n_fmbs--; > > } > > - ovs_mutex_unlock(&udpif->fmb_mutex); > > + > > udpif_drop_key_clear(udpif); > > } > > > > @@ -288,16 +259,8 @@ udpif_revalidate(struct udpif *udpif) > > struct upcall * > > upcall_next(struct udpif *udpif) > > { > > - struct upcall *next = NULL; > > - > > - ovs_mutex_lock(&udpif->upcall_mutex); > > - if (udpif->n_upcalls) { > > - udpif->n_upcalls--; > > - next = CONTAINER_OF(list_pop_front(&udpif->upcalls), struct upcall, > > - list_node); > > - } > > - ovs_mutex_unlock(&udpif->upcall_mutex); > > - return next; > > + struct list *next = guarded_list_pop_front(&udpif->upcalls); > > + return next ? CONTAINER_OF(next, struct upcall, list_node) : NULL; > > } > > > > /* Destroys and deallocates 'upcall'. */ > > @@ -316,16 +279,24 @@ upcall_destroy(struct upcall *upcall) > > struct flow_miss_batch * > > flow_miss_batch_next(struct udpif *udpif) > > { > > - struct flow_miss_batch *next = NULL; > > + for (;;) { > > + struct flow_miss_batch *next; > > + unsigned int reval_seq; > > + struct list *next_node; > > > > - ovs_mutex_lock(&udpif->fmb_mutex); > > - if (udpif->n_fmbs) { > > - udpif->n_fmbs--; > > - next = CONTAINER_OF(list_pop_front(&udpif->fmbs), > > - struct flow_miss_batch, list_node); > > + next_node = guarded_list_pop_front(&udpif->fmbs); > > + if (!next_node) { > > + return NULL; > > + } > > + > > + next = CONTAINER_OF(next_node, struct flow_miss_batch, list_node); > > + atomic_read(&udpif->reval_seq, &reval_seq); > > + if (next->reval_seq == reval_seq) { > > + return next; > > + } > > + > > + flow_miss_batch_destroy(next); > > } > > - ovs_mutex_unlock(&udpif->fmb_mutex); > > - return next; > > } > > > > /* Destroys and deallocates 'fmb'. */ > > @@ -347,52 +318,13 @@ flow_miss_batch_destroy(struct flow_miss_batch *fmb) > > free(fmb); > > } > > > > -/* Discards any flow miss batches queued up in 'udpif' for 'ofproto' > > (because > > - * 'ofproto' is being destroyed). > > - * > > - * 'ofproto''s xports must already have been removed, otherwise new flow > > miss > > - * batches could still end up getting queued. */ > > -void > > -flow_miss_batch_ofproto_destroyed(struct udpif *udpif, > > - const struct ofproto_dpif *ofproto) > > -{ > > - struct flow_miss_batch *fmb, *next_fmb; > > - > > - ovs_mutex_lock(&udpif->fmb_mutex); > > - LIST_FOR_EACH_SAFE (fmb, next_fmb, list_node, &udpif->fmbs) { > > - struct flow_miss *miss, *next_miss; > > - > > - HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &fmb->misses) { > > - if (miss->ofproto == ofproto) { > > - hmap_remove(&fmb->misses, &miss->hmap_node); > > - miss_destroy(miss); > > - } > > - } > > - > > - if (hmap_is_empty(&fmb->misses)) { > > - list_remove(&fmb->list_node); > > - flow_miss_batch_destroy(fmb); > > - udpif->n_fmbs--; > > - } > > - } > > - ovs_mutex_unlock(&udpif->fmb_mutex); > > -} > > - > > /* Retreives the next drop key which ofproto-dpif needs to process. The > > caller > > * is responsible for destroying it with drop_key_destroy(). */ > > struct drop_key * > > drop_key_next(struct udpif *udpif) > > { > > - struct drop_key *next = NULL; > > - > > - ovs_mutex_lock(&udpif->drop_key_mutex); > > - if (udpif->n_drop_keys) { > > - udpif->n_drop_keys--; > > - next = CONTAINER_OF(list_pop_front(&udpif->drop_keys), struct > > drop_key, > > - list_node); > > - } > > - ovs_mutex_unlock(&udpif->drop_key_mutex); > > - return next; > > + struct list *next = guarded_list_pop_front(&udpif->drop_keys); > > + return next ? CONTAINER_OF(next, struct drop_key, list_node) : NULL; > > } > > > > /* Destorys and deallocates 'drop_key'. */ > > @@ -410,14 +342,13 @@ void > > udpif_drop_key_clear(struct udpif *udpif) > > { > > struct drop_key *drop_key, *next; > > + struct list list; > > > > - ovs_mutex_lock(&udpif->drop_key_mutex); > > - LIST_FOR_EACH_SAFE (drop_key, next, list_node, &udpif->drop_keys) { > > + guarded_list_pop_all(&udpif->drop_keys, &list); > > + LIST_FOR_EACH_SAFE (drop_key, next, list_node, &list) { > > list_remove(&drop_key->list_node); > > drop_key_destroy(drop_key); > > - udpif->n_drop_keys--; > > } > > - ovs_mutex_unlock(&udpif->drop_key_mutex); > > } > > > > /* The dispatcher thread is responsible for receving upcalls from the > > kernel, > > @@ -618,17 +549,16 @@ recv_upcalls(struct udpif *udpif) > > upcall_destroy(upcall); > > } > > } else { > > - ovs_mutex_lock(&udpif->upcall_mutex); > > - if (udpif->n_upcalls < MAX_QUEUE_LENGTH) { > > - n_udpif_new_upcalls = ++udpif->n_upcalls; > > - list_push_back(&udpif->upcalls, &upcall->list_node); > > - ovs_mutex_unlock(&udpif->upcall_mutex); > > + size_t len; > > > > + len = guarded_list_push_back(&udpif->upcalls, > > &upcall->list_node, > > + MAX_QUEUE_LENGTH); > > + if (len > 0) { > > + n_udpif_new_upcalls = len; > > if (n_udpif_new_upcalls >= FLOW_MISS_MAX_BATCH) { > > seq_change(udpif->wait_seq); > > } > > } else { > > - ovs_mutex_unlock(&udpif->upcall_mutex); > > COVERAGE_INC(upcall_queue_overflow); > > upcall_destroy(upcall); > > } > > @@ -762,20 +692,18 @@ handle_miss_upcalls(struct udpif *udpif, struct list > > *upcalls) > > { > > struct dpif_op *opsp[FLOW_MISS_MAX_BATCH]; > > struct dpif_op ops[FLOW_MISS_MAX_BATCH]; > > - unsigned int old_reval_seq, new_reval_seq; > > struct upcall *upcall, *next; > > struct flow_miss_batch *fmb; > > size_t n_upcalls, n_ops, i; > > struct flow_miss *miss; > > > > - atomic_read(&udpif->reval_seq, &old_reval_seq); > > - > > /* Construct the to-do list. > > * > > * This just amounts to extracting the flow from each packet and > > sticking > > * the packets that have the same flow in the same "flow_miss" > > structure so > > * that we can process them together. */ > > fmb = xmalloc(sizeof *fmb); > > + atomic_read(&udpif->reval_seq, &fmb->reval_seq); > > hmap_init(&fmb->misses); > > n_upcalls = 0; > > LIST_FOR_EACH_SAFE (upcall, next, list_node, upcalls) { > > @@ -808,14 +736,10 @@ handle_miss_upcalls(struct udpif *udpif, struct list > > *upcalls) > > drop_key->key = xmemdup(dupcall->key, dupcall->key_len); > > drop_key->key_len = dupcall->key_len; > > > > - ovs_mutex_lock(&udpif->drop_key_mutex); > > - if (udpif->n_drop_keys < MAX_QUEUE_LENGTH) { > > - udpif->n_drop_keys++; > > - list_push_back(&udpif->drop_keys, &drop_key->list_node); > > - ovs_mutex_unlock(&udpif->drop_key_mutex); > > + if (guarded_list_push_back(&udpif->drop_keys, > > &drop_key->list_node, > > + MAX_QUEUE_LENGTH)) { > > seq_change(udpif->wait_seq); > > } else { > > - ovs_mutex_unlock(&udpif->drop_key_mutex); > > COVERAGE_INC(drop_queue_overflow); > > drop_key_destroy(drop_key); > > } > > @@ -868,21 +792,11 @@ handle_miss_upcalls(struct udpif *udpif, struct list > > *upcalls) > > } > > dpif_operate(udpif->dpif, opsp, n_ops); > > > > - ovs_mutex_lock(&udpif->fmb_mutex); > > - atomic_read(&udpif->reval_seq, &new_reval_seq); > > - if (old_reval_seq != new_reval_seq) { > > - /* udpif_revalidate() was called as we were calculating the > > actions. > > - * To be safe, we need to assume all the misses need revalidation. > > */ > > - ovs_mutex_unlock(&udpif->fmb_mutex); > > - flow_miss_batch_destroy(fmb); > > - } else if (udpif->n_fmbs < MAX_QUEUE_LENGTH) { > > - udpif->n_fmbs++; > > - list_push_back(&udpif->fmbs, &fmb->list_node); > > - ovs_mutex_unlock(&udpif->fmb_mutex); > > + if (guarded_list_push_back(&udpif->fmbs, &fmb->list_node, > > + MAX_QUEUE_LENGTH)) { > > seq_change(udpif->wait_seq); > > } else { > > COVERAGE_INC(fmb_queue_overflow); > > - ovs_mutex_unlock(&udpif->fmb_mutex); > > flow_miss_batch_destroy(fmb); > > } > > } > > diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h > > index 8e8264e..57d462d 100644 > > --- a/ofproto/ofproto-dpif-upcall.h > > +++ b/ofproto/ofproto-dpif-upcall.h > > @@ -36,7 +36,6 @@ struct udpif *udpif_create(struct dpif_backer *, struct > > dpif *); > > void udpif_recv_set(struct udpif *, size_t n_workers, bool enable); > > void udpif_destroy(struct udpif *); > > > > -void udpif_run(struct udpif *); > > void udpif_wait(struct udpif *); > > > > void udpif_revalidate(struct udpif *); > > @@ -105,13 +104,12 @@ struct flow_miss_batch { > > > > struct flow_miss miss_buf[FLOW_MISS_MAX_BATCH]; > > struct hmap misses; > > + > > + unsigned int reval_seq; > > }; > > > > struct flow_miss_batch *flow_miss_batch_next(struct udpif *); > > void flow_miss_batch_destroy(struct flow_miss_batch *); > > - > > -void flow_miss_batch_ofproto_destroyed(struct udpif *, > > - const struct ofproto_dpif *); > > > > /* Drop keys are odp flow keys which have drop flows installed in the > > kernel. > > * These are datapath flows which have no associated ofproto, if they did > > we > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 6f1a4e5..bf5af33 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -31,6 +31,7 @@ > > #include "dpif.h" > > #include "dynamic-string.h" > > #include "fail-open.h" > > +#include "guarded-list.h" > > #include "hmapx.h" > > #include "lacp.h" > > #include "learn.h" > > @@ -507,13 +508,8 @@ struct ofproto_dpif { > > uint64_t n_missed; > > > > /* Work queues. */ > > - struct ovs_mutex flow_mod_mutex; > > - struct list flow_mods OVS_GUARDED; > > - size_t n_flow_mods OVS_GUARDED; > > - > > - struct ovs_mutex pin_mutex; > > - struct list pins OVS_GUARDED; > > - size_t n_pins OVS_GUARDED; > > + struct guarded_list flow_mods; /* Contains "struct flow_mod"s. */ > > + struct guarded_list pins; /* Contains "struct > > ofputil_packet_in"s. */ > > }; > > > > /* By default, flows in the datapath are wildcarded (megaflows). They > > @@ -560,18 +556,11 @@ void > > ofproto_dpif_flow_mod(struct ofproto_dpif *ofproto, > > struct ofputil_flow_mod *fm) > > { > > - ovs_mutex_lock(&ofproto->flow_mod_mutex); > > - if (ofproto->n_flow_mods > 1024) { > > - ovs_mutex_unlock(&ofproto->flow_mod_mutex); > > + if (!guarded_list_push_back(&ofproto->flow_mods, &fm->list_node, > > 1024)) { > > COVERAGE_INC(flow_mod_overflow); > > free(fm->ofpacts); > > free(fm); > > - return; > > } > > - > > - list_push_back(&ofproto->flow_mods, &fm->list_node); > > - ofproto->n_flow_mods++; > > - ovs_mutex_unlock(&ofproto->flow_mod_mutex); > > } > > > > /* Appends 'pin' to the queue of "packet ins" to be sent to the controller. > > @@ -580,18 +569,11 @@ void > > ofproto_dpif_send_packet_in(struct ofproto_dpif *ofproto, > > struct ofputil_packet_in *pin) > > { > > - ovs_mutex_lock(&ofproto->pin_mutex); > > - if (ofproto->n_pins > 1024) { > > - ovs_mutex_unlock(&ofproto->pin_mutex); > > + if (!guarded_list_push_back(&ofproto->pins, &pin->list_node, 1024)) { > > COVERAGE_INC(packet_in_overflow); > > free(CONST_CAST(void *, pin->packet)); > > free(pin); > > - return; > > } > > - > > - list_push_back(&ofproto->pins, &pin->list_node); > > - ofproto->n_pins++; > > - ovs_mutex_unlock(&ofproto->pin_mutex); > > } > > > > /* Factory functions. */ > > @@ -1024,7 +1006,6 @@ process_dpif_port_error(struct dpif_backer *backer, > > int error) > > static int > > dpif_backer_run_fast(struct dpif_backer *backer) > > { > > - udpif_run(backer->udpif); > > handle_upcalls(backer); > > > > return 0; > > @@ -1286,17 +1267,8 @@ construct(struct ofproto *ofproto_) > > classifier_init(&ofproto->facets); > > ofproto->consistency_rl = LLONG_MIN; > > > > - ovs_mutex_init(&ofproto->flow_mod_mutex); > > - ovs_mutex_lock(&ofproto->flow_mod_mutex); > > - list_init(&ofproto->flow_mods); > > - ofproto->n_flow_mods = 0; > > - ovs_mutex_unlock(&ofproto->flow_mod_mutex); > > - > > - ovs_mutex_init(&ofproto->pin_mutex); > > - ovs_mutex_lock(&ofproto->pin_mutex); > > - list_init(&ofproto->pins); > > - ofproto->n_pins = 0; > > - ovs_mutex_unlock(&ofproto->pin_mutex); > > + guarded_list_init(&ofproto->flow_mods); > > + guarded_list_init(&ofproto->pins); > > > > ofproto_dpif_unixctl_init(); > > > > @@ -1422,6 +1394,7 @@ destruct(struct ofproto *ofproto_) > > struct ofputil_packet_in *pin, *next_pin; > > struct ofputil_flow_mod *fm, *next_fm; > > struct facet *facet, *next_facet; > > + struct list flow_mods, pins; > > struct cls_cursor cursor; > > struct oftable *table; > > > > @@ -1437,7 +1410,9 @@ destruct(struct ofproto *ofproto_) > > xlate_remove_ofproto(ofproto); > > ovs_rwlock_unlock(&xlate_rwlock); > > > > - flow_miss_batch_ofproto_destroyed(ofproto->backer->udpif, ofproto); > > + /* Discard any flow_miss_batches queued up for 'ofproto', avoiding a > > + * use-after-free error. */ > > + udpif_revalidate(ofproto->backer->udpif); > > > > hmap_remove(&all_ofproto_dpifs, &ofproto->all_ofproto_dpifs_node); > > > > @@ -1452,25 +1427,21 @@ destruct(struct ofproto *ofproto_) > > ovs_rwlock_unlock(&table->cls.rwlock); > > } > > > > - ovs_mutex_lock(&ofproto->flow_mod_mutex); > > - LIST_FOR_EACH_SAFE (fm, next_fm, list_node, &ofproto->flow_mods) { > > + guarded_list_pop_all(&ofproto->flow_mods, &flow_mods); > > + LIST_FOR_EACH_SAFE (fm, next_fm, list_node, &flow_mods) { > > list_remove(&fm->list_node); > > - ofproto->n_flow_mods--; > > free(fm->ofpacts); > > free(fm); > > } > > - ovs_mutex_unlock(&ofproto->flow_mod_mutex); > > - ovs_mutex_destroy(&ofproto->flow_mod_mutex); > > + guarded_list_destroy(&ofproto->flow_mods); > > > > - ovs_mutex_lock(&ofproto->pin_mutex); > > - LIST_FOR_EACH_SAFE (pin, next_pin, list_node, &ofproto->pins) { > > + guarded_list_pop_all(&ofproto->pins, &pins); > > + LIST_FOR_EACH_SAFE (pin, next_pin, list_node, &pins) { > > list_remove(&pin->list_node); > > - ofproto->n_pins--; > > free(CONST_CAST(void *, pin->packet)); > > free(pin); > > } > > - ovs_mutex_unlock(&ofproto->pin_mutex); > > - ovs_mutex_destroy(&ofproto->pin_mutex); > > + guarded_list_destroy(&ofproto->pins); > > > > mbridge_unref(ofproto->mbridge); > > > > @@ -1508,12 +1479,7 @@ run_fast(struct ofproto *ofproto_) > > return 0; > > } > > > > - ovs_mutex_lock(&ofproto->flow_mod_mutex); > > - list_move(&flow_mods, &ofproto->flow_mods); > > - list_init(&ofproto->flow_mods); > > - ofproto->n_flow_mods = 0; > > - ovs_mutex_unlock(&ofproto->flow_mod_mutex); > > - > > + guarded_list_pop_all(&ofproto->flow_mods, &flow_mods); > > LIST_FOR_EACH_SAFE (fm, next_fm, list_node, &flow_mods) { > > int error = ofproto_flow_mod(&ofproto->up, fm); > > if (error && !VLOG_DROP_WARN(&rl)) { > > @@ -1526,12 +1492,7 @@ run_fast(struct ofproto *ofproto_) > > free(fm); > > } > > > > - ovs_mutex_lock(&ofproto->pin_mutex); > > - list_move(&pins, &ofproto->pins); > > - list_init(&ofproto->pins); > > - ofproto->n_pins = 0; > > - ovs_mutex_unlock(&ofproto->pin_mutex); > > - > > + guarded_list_pop_all(&ofproto->pins, &pins); > > LIST_FOR_EACH_SAFE (pin, next_pin, list_node, &pins) { > > connmgr_send_packet_in(ofproto->up.connmgr, pin); > > list_remove(&pin->list_node); > > -- > > 1.7.10.4 > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
