Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exact Match Bulk support #226

Merged
merged 1 commit into from Mar 16, 2021

Conversation

amarsri28
Copy link
Contributor

This Patch enables Exact Match bulk support only.

Comment on lines 30 to 48
+ ValueTuple *res[32];
+ for (int i = 0; i < cnt; ) {
+ int num= (cnt-i >= 32) ?32:cnt-i;
+ uint64_t hit_mask = table_.Find(&keys[i],&res[0],num);
+
+ for(int j=i;j<num+i;j++)
+ {
+ if ((hit_mask & (1ULL << (j-i))) == 0)
+ EmitPacket(ctx, batch->pkts()[j],default_gate);
+ else
+ {
+ setValues(batch->pkts()[j], res[j-i]->action);
+ EmitPacket(ctx, batch->pkts()[j], res[j-i]->gate);
}
- gate_idx_t g = res.gate;
- EmitPacket(ctx, pkt, g);
+ }
+ i = i+num;
}
Copy link
Member

@krsna1729 krsna1729 Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to expose 32 here? Can we simplify it to be like below and move the detail all the way down to cuckooMap lookup_bulk_data API? Since at that level you can do sub-batch iteration using RTE_HASH_LOOKUP_BULK_MAX constraint

ValueTuple *res[bess::PacketBatch::kMaxBurst] = {0};
table_.Find(&keys, &res, cnt);

// Iterate results and set values and gate
for(int i = 0; i < cnt; i++)
{
    if(res[i]) {
        setValues(batch->pkts()[i], res[i]->action);
        EmitPacket(ctx, batch->pkts()[i], res[i]->gate);
    }
    else {
        EmitPacket(ctx, batch->pkts()[i], default_gate);
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a more general question NOT pertaining to this PR, The ValueTuple being pointed could be in the process of updating by the writer correct? How do we handle that scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slow path forward separate event for update , it can be taken care.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand. We have a lock for the key but no lock while accessing the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes there is no lock for accessing the value so its cleaner to handle 'update' separately .

- EmitPacket(ctx, pkt, g);
- }
+ ValueTuple *res[cnt];
+ uint64_t hit_mask = table_.Find(&keys[0],&res[0],cnt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I removed the hitmask in my example was it would make an assumption without any assert that cnt is <=64

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also when simplifying there must be some additional check in lookup_bulk_data of cuckoomap to lookup in chunks of 64 at the max in case cnt or n is more than 64

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bess already check that in pipeline for cnt .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hit_mask seems a better logic for checking presence of data(standard way , used in ut also) as compare to checking pointer null condition.

Copy link
Member

@krsna1729 krsna1729 Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need for hit_mask being exposed all the way up to exact_match.cc? cant we simply use pointer being null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointer would be easier to check...i gave it a thought, but we dnt want to waste cycles in initializing pointer array to zero everytime , also didnt got any dpdk reference which says that for 'not present' keys , pointer would be made null by dpdk(though it might be doing in this dpdk version) but dnt want to be dependent on internal dpdk logic for future dpdk revisions.
Unit test also uses same hit_msk logic for checking presence.

@krsna1729
Copy link
Member

  1. rebase
  2. Run clang-format (contributing) manually or automatically as part of git hooks before committing and generating the BESS patch

- EmitPacket(ctx, pkt, g);
- }
+ ValueTuple *res[cnt];
+ uint64_t hit_mask = table_.Find(&keys[0],&res[0],cnt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be simplified

table_.Find(&keys[0],&res[0],cnt);

to

table_.Find(keys, res, cnt);

@krsna1729 krsna1729 merged commit 6060980 into omec-project:master Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants