On 12/7/25 10:14 PM, Alexandra Rukomoinikova wrote:
> From Open vSwitch 2.10 onwards, dp_hash is the default selection method
> for select groups. When this method is used, packet recirculation occurs
> to compute the hash at the datapath level. Previously, this case was
> logged as "no live bucket", which was misleading.
Hi. Thanks for the patch! This is indeed an annoying issue.
I'd say the area of this patch in the subject should be ofproto-dpif-xlate
instead of ofproto-dpif-trace, but tracing and the dp_hash should be
somehow mentioned.
And we'll need a fixes tag:
Fixes: 53cc166ae5fe ("xlate: Use dp_hash for select groups.")
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-December/052860.html
> Signed-off-by: Alexandra Rukomoinikova <[email protected]>
> ---
> ofproto/ofproto-dpif-xlate.c | 26 +++++++++++++++++++++-----
> tests/ofproto-dpif.at | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 2c8197fb7..7437c077a 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4973,7 +4973,8 @@ pick_hash_fields_select_group(struct xlate_ctx *ctx,
> struct group_dpif *group)
> }
>
> static struct ofputil_bucket *
> -pick_dp_hash_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
> +pick_dp_hash_select_group(struct xlate_ctx *ctx, struct group_dpif *group,
> + bool *is_dp_hash_recirc)
> {
> uint32_t dp_hash = ctx->xin->flow.dp_hash;
>
> @@ -4988,6 +4989,7 @@ pick_dp_hash_select_group(struct xlate_ctx *ctx, struct
> group_dpif *group)
> hash_alg = OVS_HASH_ALG_L4;
> }
> ctx_trigger_recirculate_with_hash(ctx, hash_alg, group->hash_basis);
> + *is_dp_hash_recirc = true;
I'm not sure we need to pass around yet another flag. We already have all the
required information in the context structure. We know if we're freezing the
translation and the only reason for this would be recirculation for the select
group. And it's probably better to log that we're recirculating right here,
so it's easier to find the place in the code from the trace.
> return NULL;
> } else {
> uint32_t hash_mask = group->hash_mask;
> @@ -5011,7 +5013,8 @@ pick_dp_hash_select_group(struct xlate_ctx *ctx, struct
> group_dpif *group)
> }
>
> static struct ofputil_bucket *
> -pick_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
> +pick_select_group(struct xlate_ctx *ctx, struct group_dpif *group,
> + bool *is_dp_hash_recirc)
> {
> /* Select groups may access flow keys beyond L2 in order to
> * select a bucket. Recirculate as appropriate to make this possible.
> @@ -5029,7 +5032,7 @@ pick_select_group(struct xlate_ctx *ctx, struct
> group_dpif *group)
> return pick_hash_fields_select_group(ctx, group);
> break;
> case SEL_METHOD_DP_HASH:
> - return pick_dp_hash_select_group(ctx, group);
> + return pick_dp_hash_select_group(ctx, group, is_dp_hash_recirc);
> break;
> default:
> /* Parsing of groups ensures this never happens */
> @@ -5039,6 +5042,18 @@ pick_select_group(struct xlate_ctx *ctx, struct
> group_dpif *group)
> return NULL;
> }
>
> +static void
> +xlate_group_action_report(struct xlate_ctx *ctx,
> + bool is_dp_hash_recirc)
> +{
> + if (is_dp_hash_recirc) {
> + xlate_report(ctx, OFT_DETAIL,
> + "selection method in use: dp_hash, recirculating");
> + } else {
> + xlate_report(ctx, OFT_DETAIL, "no live bucket");
> + }
> +}
> +
> static void
> xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group,
> bool is_last_action)
> @@ -5053,8 +5068,9 @@ xlate_group_action__(struct xlate_ctx *ctx, struct
> group_dpif *group,
> xlate_group_stats(ctx, group, NULL);
> } else {
> struct ofputil_bucket *bucket;
> + bool is_dp_hash_recirc = false;
> if (group->up.type == OFPGT11_SELECT) {
> - bucket = pick_select_group(ctx, group);
> + bucket = pick_select_group(ctx, group, &is_dp_hash_recirc);
> } else if (group->up.type == OFPGT11_FF) {
> bucket = pick_ff_group(ctx, group);
> } else {
> @@ -5067,7 +5083,7 @@ xlate_group_action__(struct xlate_ctx *ctx, struct
> group_dpif *group,
> xlate_group_bucket(ctx, bucket, is_last_action);
> xlate_group_stats(ctx, group, bucket);
> } else {
> - xlate_report(ctx, OFT_DETAIL, "no live bucket");
> + xlate_group_action_report(ctx, is_dp_hash_recirc);
Here we could just check if we're in the process of freezing the flow
translation
(see ctx->freezing) and avoid logging in this case, as there is no reason for us
to have a bucket here if we're not going to use it.
> if (ctx->xin->xcache) {
> ofproto_group_unref(&group->up);
> }
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 7ebbee56d..794e3b06a 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -13328,3 +13328,34 @@ AT_CHECK([ovs-appctl coverage/read-counter
> revalidate_missing_dp_flow], [0],
>
> OVS_VSWITCHD_STOP(["/failed to flow_del (No such file or directory)/d"])
> AT_CLEANUP
> +
> +
nit: One empty line is enough.
> +AT_SETUP([ofproto-dpif - dp_hash action tracing])
I'd suggest moving this test higher to other tests for select groups.
Most of them have "ofproto-dpif - select group ..." names, so this one may
also be re-named to reflect that it checks tracing of select groups with
dp_hash selection method.
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 \
> + 'group_id=1,type=select,bucket=bucket_id:0,weight:10,actions=output:p1'])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 \
> +
> 'in_port=3,dl_src=de:9c:43:99:7c:46,dl_dst=ce:5c:1e:cd:2b:12,actions=group:1'])
There is no port with an ofport number 3. This sort of works, because
OVS doesn't really check if the input port exists as long as there is
a match in the classifier, but we should be using one of the existing
ports instead. You may also use names to avoid any potential issues,
i.e. in_port=p1. Same for the port 4 and all the commands below.
Also, looks like we're using 50:54:00:00:00:XX mac addresses throughout
this file for the most part, so we may use them here as well, just for
consistency. We're not testing the classifier here. It's also not
necessary to match on any addresses, IIRC, but we can keep them, it's fine.
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 \
> +
> in_port=3,tcp,dl_src=de:9c:43:99:7c:46,dl_dst=ce:5c:1e:cd:2b:12,nw_src=10.2.0.2,nw_dst=10.2.0.3
> | sed -n '7p'], [0], [dnl
Relying on this line to be the 7th line is fairly brittle. It may be
better to grep for '\->' instead. Same for the next check.
> + -> selection method in use: dp_hash, recirculating
> +])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-group br0 \
> + 'group_id=2,type=select,selection_method=dp_hash'])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 \
> +
> 'in_port=4,dl_src=de:9c:43:99:7c:46,dl_dst=ce:5c:1e:cd:2b:12,actions=group:2'])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 \
> +
> in_port=4,tcp,dl_src=de:9c:43:99:7c:46,dl_dst=ce:5c:1e:cd:2b:12,nw_src=10.2.0.2,nw_dst=10.2.0.3
> | sed -n '7p'], [0], [dnl
> + -> no live bucket
> +])
> +
> +
nit: One empty line is enough.
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
