On Tue, May 20, 2014 at 01:25:35PM -0700, Jarno Rajahalme wrote:
>
> On May 20, 2014, at 1:03 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > On Tue, May 20, 2014 at 12:40:20PM -0700, Jarno Rajahalme wrote:
> >>
> >> On May 20, 2014, at 10:08 AM, Ben Pfaff <b...@nicira.com> wrote:
> >>
> >>> On Tue, May 20, 2014 at 09:12:23AM -0700, Ben Pfaff wrote:
> >>>> On Fri, May 16, 2014 at 04:38:02PM -0700, Jarno Rajahalme wrote:
> >>>>>> +static bool
> >>>>>> +cmap_insert_dup(struct cmap_node *new_node, uint32_t hash,
> >>>>>> + struct cmap_bucket *b)
> >>>>>> +{
> >>>>>> + int i;
> >>>>>> +
> >>>>>> + for (i = 0; i < CMAP_K; i++) {
> >>>>>> + struct cmap_node *node = b->nodes[i];
> >>>>>> + if (b->hashes[i] == hash && node) {
> >>>>>> + for (;;) {
> >>>>>> + struct cmap_node *next =
> >>>>>> cmap_node_next_protected(node);
> >>>>>> + if (!next) {
> >>>>>> + ovsrcu_set(&node->next, new_node);
> >>>>>> + return true;
> >>>>>> + }
> >>>>>> + node = next;
> >>>>>> + }
> >>>>>
> >>>>> It would be faster to add the dup to the beginning of the list instead:
> >>>>>
> >>>>> for (i = 0; i < CMAP_K; i++) {
> >>>>> if (b->hashes[i] == hash) {
> >>>>> ovsrcu_set(&new_node->next, b->nodes[i]);
> >>>>> b->nodes[i] = new_node;
> >>>>> return true;
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> Syncing via the counter should be unnecessary, as the hash value was
> >>>>> already set.
> >>>>
> >>>> That's a good point about the counter. That's the reason that I
> >>>> switched to this method in v2.
> >>>
> >>> OK, here's an incremental I'm folding in. What do you think?
> >>>
> >>> diff --git a/lib/cmap.c b/lib/cmap.c
> >>> index a1367bc..30a6e2d 100644
> >>> --- a/lib/cmap.c
> >>> +++ b/lib/cmap.c
> >>> @@ -392,7 +392,7 @@ cmap_set_bucket(struct cmap_bucket *b, int i,
> >>>
> >>> /* Searches 'b' for a node with the given 'hash'. If it finds one, adds
> >>> * 'new_node' to the node's linked list and returns true. If it does not
> >>> find
> >>> - * one, returns false. */
> >>> + * one, returns false. */
> >>> static bool
> >>> cmap_insert_dup(struct cmap_node *new_node, uint32_t hash,
> >>> struct cmap_bucket *b)
> >>> @@ -402,14 +402,30 @@ cmap_insert_dup(struct cmap_node *new_node,
> >>> uint32_t hash,
> >>> for (i = 0; i < CMAP_K; i++) {
> >>> struct cmap_node *node = b->nodes[i];
> >>> if (b->hashes[i] == hash && node) {
> >>> + struct cmap_node *p;
> >>> +
> >>> + /* The common case is that 'new_node' is a singleton, with a
> >>> null
> >>> + * 'next' pointer, but rehashing can add a longer chain.
> >>> Find the
> >>> + * end of the chain starting at 'new_node', then splice
> >>> 'node' to
> >>> + * the end of that chain. */
> >>
> >> I think this could be done regardless of the value of the
> >> ?node?. ?node? could be NULL, if a preceding remove just NULLed the
> >> node pointer but left the hash intact. I.e. it seems to me that when
> >> a node is removed, there is no reason to remove the hash value from
> >> the bucket, meaning that syncing via the counter would not be
> >> necessary when removing nodes from cmap.
> >
> > I believe that the incremental that I posted does what you are describing.
> > It does not call cmap_set_bucket() or set ->hashes[].
>
> There is still this before the added code lines:
>
> @@ -402,14 +402,30 @@ cmap_insert_dup(struct cmap_node *new_node, uint32_t
> hash,
> for (i = 0; i < CMAP_K; i++) {
> struct cmap_node *node = b->nodes[i];
> if (b->hashes[i] == hash && node) {
>
> i.e., ?node? is checked to be non-NULL.
Oh, I completely missed your meaning before.
I changed this code to:
if (b->hashes[i] == hash) {
if (node) {
struct cmap_node *p;
/* The common case is that 'new_node' is a singleton, with a
* null 'next' pointer, but rehashing can add a longer chain.
* Find the end of the chain starting at 'new_node', then
* splice 'node' to the end of that chain. */
p = new_node;
for (;;) {
struct cmap_node *next = cmap_node_next_protected(p);
if (!next) {
break;
}
p = next;
}
ovsrcu_set(&p->next, node);
} else {
/* The hash value is there from some previous insertion, but
* the associated node has been removed. We're not really
* inserting a duplicate, but we can still reuse the slot.
* Carry on. */
}
/* Change the bucket to point to 'new_node'. This is a degenerate
* form of cmap_set_bucket() that doesn't update the counter since
* we're only touching one field and in a way that doesn't change
* the bucket's meaning for readers. */
b->nodes[i] = new_node;
atomic_thread_fence(memory_order_release);
return true;
}
and will now re-test.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
