Ok, after long last, I was able to get my perf environment to work. Here are
the results for the TCP_CRR (300 flows on server209/210 to be exact) test with
master with and without the flow hash table in up call.
The mean and median transmissions/second are 2-3 lower without the hash table;
I ran the test a few times to confirm.
Let me know if this is a significant performance drop. If not, I'll submit
another version. If so, we likely shouldn't commit this patch.
Also, the logs didn't seem to have any unexpected warning or errors from the
handlers with respect to duplicate flow additions.
With hash map in upcall:
NUM RESULTS: 23944
MEAN: 150.843937
MEDIAN: 150.660000
Without hash map in up call:
NUM RESULTS: 24736
MEAN: 147.618262
MEDIAN: 147.300000
Ryan Wilson
Member of Technical Staff
wr...@vmware.com
3401 Hillview Avenue, Palo Alto, CA
650.427.1511 Office
916.588.7783 Mobile
On May 19, 2014, at 2:05 PM, Alex Wang <al...@nicira.com> wrote:
> Thanks Ryan, this is a great refactoring.
>
> Looks good to me,
>
> Minor issues:
>
> 1. Could you rebase the patch against master? Need to fix some new calls,
> added after you posted the patch.
>
>
>
> On Wed, May 7, 2014 at 3:14 PM, Ryan Wilson <wr...@nicira.com> wrote:
> The upcall hander keeps a hash table which hashes flow to a list of
> corresponding packets.
>
>
> s/hander/handler
>
>
>
>
> @@ -710,62 +687,55 @@ compose_slow_path(struct udpif *udpif, struct xlate_out
> *xout,
> odp_put_userspace_action(pid, &cookie, sizeof cookie.slow_path, buf);
> }
>
> -static struct flow_miss *
> -flow_miss_find(struct hmap *todo, const struct ofproto_dpif *ofproto,
> - const struct flow *flow, uint32_t hash)
> +static void
> +upcall_init(struct upcall *upcall, struct flow *flow, struct ofpbuf *packet,
> + struct ofproto_dpif *ofproto, struct dpif_upcall *dupcall,
> + odp_port_t odp_in_port)
> {
> - struct flow_miss *miss;
> -
> - HMAP_FOR_EACH_WITH_HASH (miss, hmap_node, hash, todo) {
> - if (miss->ofproto == ofproto && flow_equal(&miss->flow, flow)) {
> - return miss;
> - }
> + struct xlate_in xin;
> + struct pkt_metadata md = pkt_metadata_from_flow(flow);
>
>
> + flow_extract(packet, &md, &upcall->flow);
> +
>
>
> Add a newline between local variable declaration and the code.
>
>
>
> +
>
> /* Do not install a flow into the datapath if:
> *
> * - The datapath already has too many flows.
> *
> - * - An earlier iteration of this loop already put the same flow.
> - *
> * - We received this packet via some flow installed in the kernel
> * already. */
> if (may_put
> - && !miss->put
> && upcall->dpif_upcall.type == DPIF_UC_MISS) {
> struct ofpbuf mask;
> bool megaflow;
>
> - miss->put = true;
> -
>
>
> Here, the removal of 'miss->put', may cause the warning of inserting
> duplicated flows (when upcalls from same flow
> at handled in same batch). We think it is okay, to have this warning, since
> it should be very rare and it is will not
> cause duplicated flows in datapath. Let's see if there is anything shown up
> during the tcp_crr test.
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbffg%3D%3D%0A&m=%2Bt0AOhT%2BUeh9KvK2K63%2Bz2ztZ6dUP5BWXcW%2Fcklreyk%3D%0A&s=eae05e79932e5ef2a2dd8c70589071e6c7a47b0f40b0b5a890c9c1ddc74c0df9
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
