On 5/13/24 12:32 PM, Eelco Chaudron wrote:
On 10 May 2024, at 13:15, Adrian Moreno wrote:
On 5/10/24 12:06 PM, Eelco Chaudron wrote:
On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
When a OFP_SAMPLE action is xlated and a dpif_psample object has been
configured (via Flow_Sample_Collector_Set table) with the same
collector_set_id, add psample information to the odp sample action.
See comments below.
//Eelco
<SNIP>
- compose_sample_action(ctx, probability, &cookie, tunnel_out_port, false);
+static void
+xlate_sample_action(struct xlate_ctx *ctx,
+ const struct ofpact_sample *os)
+{
+ struct dpif_psample *psample = ctx->xbridge->psample;
+ struct dpif_ipfix *ipfix = ctx->xbridge->ipfix;
+ struct user_action_cookie *upcall_cookie = NULL;
+ struct psample_args *psample_args = NULL;
+ odp_port_t tunnel_out_port = ODPP_NONE;
+ uint32_t group_id;
+
+ if (!ipfix && !psample) {
+ return;
+ }
+
+ /* Scale the probability from 16-bit to 32-bit while representing
+ * the same percentage. */
+ uint32_t probability =
+ ((uint32_t) os->probability << 16) | os->probability;
+
+ if (ipfix) {
+ upcall_cookie = xzalloc(sizeof *upcall_cookie);
+ xlate_fill_ipfix_sample(ctx, os, ipfix, upcall_cookie,
+ &tunnel_out_port);
+ }
+ if (psample) {
+ if (dpif_psample_get_group_id(psample,
See earlier lock contention concern on taking the mutex() for parallel upcalls.
+ os->collector_set_id,
+ &group_id)) {
+ psample_args = xzalloc(sizeof *psample_args);
+ ofpbuf_init(&psample_args->cookie, OVS_PSAMPLE_COOKIE_MAX_SIZE);
+
+ psample_args->group_id = group_id;
+ ofpbuf_put(&psample_args->cookie, &os->obs_domain_id,
+ sizeof(os->obs_domain_id));
+ ofpbuf_put(&psample_args->cookie, &os->obs_point_id,
+ sizeof(os->obs_point_id));
+ }
+ }
+
+ compose_sample_action(ctx, probability, upcall_cookie, psample_args,
+ tunnel_out_port, false);
+
+ if (upcall_cookie) {
+ free(upcall_cookie);
+ }
+ if (psample_args) {
+ ofpbuf_uninit(&psample_args->cookie);
+ free(psample_args);
+ }
Can we avoid these small memory allocations?
Probably. I'll try to rework it. Two of the arguments to compose_sample_action
are now optional so having a pointer to the stack will not work unless we add
flags to indicate whether they are valid or not. But maybe we can rework it by
having something like the follwoing to avoid clogging the argument list;
struct sample_args {
struct user_action_cookie *upcall;
struct psample_args *psample;
bool has_psample;
bool has_upcall;
};
Don’t think we need a structure, we can use your existing bools. Something like
the below seems simple enough:
Ok, yes that's a good alternative. I'll add it. Thanks
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a9856e358..42100f15b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5958,8 +5958,8 @@ xlate_sample_action(struct xlate_ctx *ctx,
{
struct dpif_psample *psample = ctx->xbridge->psample;
struct dpif_ipfix *ipfix = ctx->xbridge->ipfix;
- struct user_action_cookie *upcall_cookie = NULL;
- struct psample_args *psample_args = NULL;
+ struct user_action_cookie upcall_cookie;
+ struct psample_args psample_args;
odp_port_t tunnel_out_port = ODPP_NONE;
uint32_t group_id;
@@ -5973,21 +5973,20 @@ xlate_sample_action(struct xlate_ctx *ctx,
((uint32_t) os->probability << 16) | os->probability;
if (ipfix) {
- upcall_cookie = xzalloc(sizeof *upcall_cookie);
- xlate_fill_ipfix_sample(ctx, os, ipfix, upcall_cookie,
+ xlate_fill_ipfix_sample(ctx, os, ipfix, &upcall_cookie,
&tunnel_out_port);
}
if (psample) {
if (dpif_psample_get_group_id(psample,
os->collector_set_id,
&group_id)) {
- psample_args = xzalloc(sizeof *psample_args);
- ofpbuf_init(&psample_args->cookie, OVS_PSAMPLE_COOKIE_MAX_SIZE);
- psample_args->group_id = group_id;
- ofpbuf_put(&psample_args->cookie, &os->obs_domain_id,
+ ofpbuf_init(&psample_args.cookie, OVS_PSAMPLE_COOKIE_MAX_SIZE);
+
+ psample_args.group_id = group_id;
+ ofpbuf_put(&psample_args.cookie, &os->obs_domain_id,
sizeof(os->obs_domain_id));
- ofpbuf_put(&psample_args->cookie, &os->obs_point_id,
+ ofpbuf_put(&psample_args.cookie, &os->obs_point_id,
sizeof(os->obs_point_id));
if (ctx->xin->xcache) {
@@ -5997,19 +5996,15 @@ xlate_sample_action(struct xlate_ctx *ctx,
entry->psample.psample = dpif_psample_ref(psample);
entry->psample.collector_set_id = os->collector_set_id;
}
-
}
}
- compose_sample_action(ctx, probability, upcall_cookie, psample_args,
- tunnel_out_port, false);
+ compose_sample_action(ctx, probability, ipfix ? &upcall_cookie : NULL,
+ psample ? &psample_args : NULL, tunnel_out_port,
+ false);
- if (upcall_cookie) {
- free(upcall_cookie);
- }
- if (psample_args) {
- ofpbuf_uninit(&psample_args->cookie);
- free(psample_args);
+ if (psample) {
+ ofpbuf_uninit(&psample_args.cookie);
}
}
<SNIP>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
