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:
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
