Hi Antonio,
In this patch of the patchset there are three lines removed from the direct
command flow:
- miniflow_extract(packet, &key->mf);
- key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
- flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);
Which are then replicated in several different branches for logic. This is a
lot of duplication of logic.
I *think* (I haven't tested it) this can be re-written with less branching like
this:
if (!md_is_valid) {
pkt_metadata_init(&packet->md, port_no);
}
miniflow_extract(packet, &key->mf);
if (OVS_LIKELY(cur_min)) {
if (md_is_valid) {
key->hash = dpif_netdev_packet_get_rss_hash(packet,
&key->mf);
}
else
{
if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
key->hash = dp_packet_get_rss_hash(packet);
} else {
key->hash = miniflow_hash_5tuple(&key->mf, 0);
dp_packet_set_rss_hash(packet, key->hash);
}
flow = emc_lookup(flow_cache, key);
}
} else {
flow = NULL;
}
Also if I'm understanding correctly the final effect of the patch is that in
the case where !md_is_valid it effectively replicates the work of
dpif_netdev_packet_get_rss_hash() but leaving out the if (recirc_depth) block
of that fn. This is effectively overriding the return value of
recirc_depth_get_unsafe in dpif_netdev_packet_get_rss_hash() and
forcing/assuming that it is zero.
If so it would be less disturbing to the existing code to just add a bool arg
to dpif_netdev_packet_get_rss_hash() called do_not_check_recirc_depth and use
that to return early (before the if (recirc_depth) check). Also in that case
the patch would require none of the conditional logic changes (neither the
original or that suggested in this email) and should be able to just set the
proposed do_not_check_recirc_depth based on md_is_valid.
Also this is showing up as a patch set can you add a cover letter to outline
the overall goal of the patchset.
Thanks,
Billy.
> -----Original Message-----
> From: [email protected] [mailto:ovs-dev-
> [email protected]] On Behalf Of [email protected]
> Sent: Monday, June 19, 2017 11:12 AM
> To: [email protected]
> Subject: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash when
> EMC is disabled.
>
> From: Antonio Fischetti <[email protected]>
>
> When EMC is disabled the reading of RSS hash is skipped.
> For packets that are not recirculated it retrieves the hash value without
> considering the recirc id.
>
> This is mostly a preliminary change for the next patch in this series.
>
> Signed-off-by: Antonio Fischetti <[email protected]>
> ---
> In our testbench we used monodirectional traffic with 64B UDP packets PDM
> threads: 2 Traffic gen. streams: 1
>
> we saw the following performance improvement:
>
> Orig 11.49 Mpps
> With Patch#1: 11.62 Mpps
>
> lib/dpif-netdev.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 02af32e..fd2ed52
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4584,13 +4584,33 @@ emc_processing(struct dp_netdev_pmd_thread
> *pmd,
>
> if (!md_is_valid) {
> pkt_metadata_init(&packet->md, port_no);
> + miniflow_extract(packet, &key->mf);
> + /* This is not a recirculated packet. */
> + if (OVS_LIKELY(cur_min)) {
> + /* EMC is enabled. We can retrieve the 5-tuple hash
> + * without considering the recirc id. */
> + if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
> + key->hash = dp_packet_get_rss_hash(packet);
> + } else {
> + key->hash = miniflow_hash_5tuple(&key->mf, 0);
> + dp_packet_set_rss_hash(packet, key->hash);
> + }
> + flow = emc_lookup(flow_cache, key);
> + } else {
> + /* EMC is disabled, skip emc_lookup. */
> + flow = NULL;
> + }
> + } else {
> + /* Recirculated packets. */
> + miniflow_extract(packet, &key->mf);
> + if (OVS_LIKELY(cur_min)) {
> + key->hash = dpif_netdev_packet_get_rss_hash(packet, &key-
> >mf);
> + flow = emc_lookup(flow_cache, key);
> + } else {
> + flow = NULL;
> + }
> }
> - miniflow_extract(packet, &key->mf);
> key->len = 0; /* Not computed yet. */
> - key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
> -
> - /* If EMC is disabled skip emc_lookup */
> - flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);
> if (OVS_LIKELY(flow)) {
> dp_netdev_queue_batches(packet, flow, &key->mf, batches,
> n_batches);
> --
> 2.4.11
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
