From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7BA653537E8 for ; Fri, 12 Jun 2026 17:55:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781286906; cv=none; b=c1pTaTmSna2U47okDFVghYKpgNnVyo/XlCCdAis682VQ0+wxIE9gEOkFlRdyyBJNjx7475lJYGE/l9cAGaxi/CAyKzrICp8ZYhn3NOeUOJPw6NCnDO+izrvOGdaX7BJSSR8drlY7tiJfACTXc7rXKskcISqlqB13b6KJ3bNlIDo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781286906; c=relaxed/simple; bh=cpXy/5c5CDC22j4D+5veOeHsXhdNWQ7BkuZSci8x89Q=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=HcoAomXhwBjSZm7x/i8W7JEeoEp/lmU3N1sPiHYLx9flpjCQef+LsIFe5CeNJZ0h8kKwDWHKfBFHRaQJAic8LGJ0xvcPx7q19DvGhnxo5gpSRGRPCJP3obtq/FXrwHqcQo+/LuOnIFh1IaXZ0EdpN2cPG7wrLe5Jk7XPnfIqxEI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=EhaLm/DS; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=fI+ICkcu; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="EhaLm/DS"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="fI+ICkcu" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781286903; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pXDhvV8bnaAuLCQ67j/U6pOfdL+WZT0Eo2w9Zx37nwM=; b=EhaLm/DSz1vGR3g8/VuUz8h9tNIrynqrgTGjnXUcqANt/frrshJssdEN2zktPpu6mMeEsF 5tVOcQnkpk2g8PZtANZ8Hxkfm+nRn9OMcgfzo1QEK+1c3KDbE/7BYjFmZ6e8Nqhq0SBDxV Ifad1uXeaODNgjQdEK/Nnnd2njqfr9Q= Received: from mail-oo1-f69.google.com (mail-oo1-f69.google.com [209.85.161.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-49-_LxGFacMPmGbfEqrid4oXQ-1; Fri, 12 Jun 2026 13:55:02 -0400 X-MC-Unique: _LxGFacMPmGbfEqrid4oXQ-1 X-Mimecast-MFC-AGG-ID: _LxGFacMPmGbfEqrid4oXQ_1781286901 Received: by mail-oo1-f69.google.com with SMTP id 006d021491bc7-69e45c65b5eso1358038eaf.2 for ; Fri, 12 Jun 2026 10:55:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1781286901; x=1781891701; darn=vger.kernel.org; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject :date:message-id:reply-to; bh=pXDhvV8bnaAuLCQ67j/U6pOfdL+WZT0Eo2w9Zx37nwM=; b=fI+ICkcu2zYyetbydHYvAOzHh4NOnM7MYK52DwhoUpwgO92ZMIXgk1AAggq26iZuCb 4NS9lXJYe3SavrjmpfBJZ0jBGf6q/qtheFg/HieUVf4MR7X9PPs8bVewiHZBYsh/S76B HoxQo7lGOSQsWLR147dS2tg5QocQJTAP+cG80iVnJRxxCrAsspS1XrrH1IOf1pdaZ1PT yJPPyMxSMmpK64z15ig3k/XdoYdRlqSRM2X4Gj4ZivYwed6USrUrIsHgWiKp8vzwPPry knncuiaKA6ifpEE9noF3e8Vr459ZrVV6ZNEaS9KRtuqEzqWjtLBvvqAXaEjdP9Zyg5+B sfZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781286901; x=1781891701; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=pXDhvV8bnaAuLCQ67j/U6pOfdL+WZT0Eo2w9Zx37nwM=; b=LQEnOGKSYB7H0VvfA5LUEGjvFrVtLA7+0lw9Wo0nagwSNPW27TGJFyA22345oODBiX r9sBBAtsWFvidxfQaylPZmAwrQFuCXi1aVXAmp0Rl4rowv9PF9g3v/LzPbMzgSTa80Pw 1U0qHzWZ/jKRfZ+J+61eDV8NoZ2txx3ciN/XwGvV6hMGCdfSWDt3f7IGsMYCunagB7PV YH2IF8AoD9MWNzYCINzQnlltbMWTHBjvvqGZvsM+XeY+EyEjPuY2GZGoC4EdaFtmeKdh PrV1FpWbjiJOZlfJ9O/oPzoyd2Lt6MnKo9W28csnuataDQBP/zWb7vOxAIGU+dD6HxEo gyxA== X-Forwarded-Encrypted: i=1; AFNElJ9R2qtCNSRYdyXjvvaO3oY3rsBnrdY9a3pFY+Ej+zoSogJ83SHj2G0yyWtLSAvopei2A8u1yfkbLUoP8Bc=@vger.kernel.org X-Gm-Message-State: AOJu0YyP8uTsN3VS7U+kpj3JEd0wWjbSGAgPS2x52hYo5Elo7IqL+0Iv CS6L7T32ReTeqd76NrB0bvgUYui8C4Zym3pUKkOOq5W4YpcmAGEYwxtRH34aD84C3gN3u7jVYcN 6CcumGGw2YjhmEYlbFn6NrA5V7r+XqfcKMUzwZdTBpWJ9vl7emzItAyFH64V92luhRA== X-Gm-Gg: Acq92OEdtUUCarKb7+T4LyWcdr0L5bdEeGm16zo4mS2HaH+weXebZ52wXocMBI0/OXD UUVgBbPKUiRvkI7JeJl1l1AW57g7dHf7eICXKX2hSMTK3g1/Qt+UutQgXb6oKqo9dV3BJ6P/DdJ PBUa8m8SIOfR9nx00VQmJDOkRc54Wlt2gAdzw+/wwHki+2euGNN5JqxwnlsOJ+Jq26QxIP1NRj9 kps03QS/9/xdqjBSlJpORB6i70Mi8PY7Oaid4uHaEv6CmkfE9+jS1uR3jUdQVQpx/vTawxUekQJ Q+HDvc23gdj72x6SeaSPv9yigHATZHmuh9Nxs2NGi9T88iIkA29/k0dwQGvd/Jq/a161VV2GZ6M aSLfjhz8VTZJZ65z3TQhUAraEvC+g+FbeQlfmtdlRHY5GsCd1pbxHtw== X-Received: by 2002:a4a:e915:0:b0:69e:b6df:6824 with SMTP id 006d021491bc7-69edc5e9a07mr2194830eaf.6.1781286901393; Fri, 12 Jun 2026 10:55:01 -0700 (PDT) X-Received: by 2002:a4a:e915:0:b0:69e:b6df:6824 with SMTP id 006d021491bc7-69edc5e9a07mr2194809eaf.6.1781286900853; Fri, 12 Jun 2026 10:55:00 -0700 (PDT) Received: from crwood-thinkpadp16vgen1.minnmso.csb ([2601:447:cc01:6890:c623:cd89:345e:99f3]) by smtp.gmail.com with ESMTPSA id 006d021491bc7-69ed8369957sm2041681eaf.3.2026.06.12.10.54.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 12 Jun 2026 10:55:00 -0700 (PDT) Message-ID: <725da8a4ca6a46ea8ba41778971602b49d650e60.camel@redhat.com> Subject: Re: [PATCH] rtla: Simplify osnoise tracer option setting code From: Crystal Wood To: Tomas Glozar , Steven Rostedt Cc: John Kacur , Luis Goncalves , Costa Shulyupin , Wander Lairson Costa , LKML , linux-trace-kernel Date: Fri, 12 Jun 2026 12:54:59 -0500 In-Reply-To: <20260612115121.54862-1-tglozar@redhat.com> References: <20260612115121.54862-1-tglozar@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.60.1 (3.60.1-1.fc44) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Fri, 2026-06-12 at 13:51 +0200, Tomas Glozar wrote: > Each osnoise tracer option (in /sys/kernel/tracing/osnoise) used by RTLA > requires four functions to be defined: >=20 > - static osnoise_get_() - to get the current value of the option > and save it into struct osnoise_context's orig_ field, > - osnoise_set_() - to set the value of the option requested by the > user after reading and saving the original with osnoise_get_(), > and save it into field of struct osnoise_context, > - osnoise_restore_() - restore the value recorded in orig_, > - static osnoise_put_() - restore the value recorded in orig_ > and update to reflect that. >=20 > The logic is duplicated for all the options, except for cpus (which is > the only string option) and period/runtime (which are handled together > and feature extra checks). Thanks for taking this on... this is one of the things that was bugging me during consolidation work that I didn't get to. > Deduplicate the logic using a set of macros featuring the X macro > pattern, defined in src/common.h: >=20 > - OSNOISE_LL_OPTIONS, which invokes OSNOISE_LL_OPTION macro for all > "long long" options, > - OSNOISE_FLAG_OPTIONS, which invokes OSNOISE_FLAG_OPTION macro for all > flag (boolean values in osnoise/options file) options. >=20 > The list macros are then invoked in four places: >=20 > - for struct osnoise_context fields in src/common.h, > - for function declarations, moved into src/common.h from > src/osnoise.h, > - for function definitions in src/osnoise.c, > - for context initialization and restoration, in osnoise_context_alloc() > and osnoise_put_context(), both in src/osnoise.c. >=20 > OSNOISE_LL_OPTIONS takes three options: name - struct osnoise_context > field name (written "" above), path - filename inside > /sys/kernel/tracing/osnoise passed to libtracefs, and init_val - initial > value of struct fields, corresponding to an otherwise invalid option > (some options use OSNOISE_OPTION_INIT_VAL =3D -1, some use > OSNOISE_TIME_INIT_VAL =3D 0). Can we simplify by always using -1? Especially since that's already treated as the universal "invalid" by osnoise_read_ll_config(). FWIW using "init val" to mean "invalid" rather than "default" is a bit unintuitive. > OSNOISE_FLAG_OPTION is similar, but instead of path, it takes the option > string inside /sys/kernel/tracing/osnoise/options (opt_string), and no > init_val, as it is purely boolean (0 or 1). >=20 > Previously, for options timerlat_align and osnoise_workload, the return > value of osnoise_set_() distinguished between -2 (option cannot be > set) and -1 (option not present). This distinction is expanded for all > options for consistency; for most options, it is currently not used, > only osnoise_workload is implemented to avoid error on -1 on older RTLA > versions. "on -1 on"? > The change overall has two main benefits: it makes it much simpler to > add a new option, as well as to change existing logic consistently for > all of them. It also makes the code shorter by a bit over 500 lines. >=20 > There is no intentional user-visible change coming from the refactoring. > osnoise_restore_() for flag options now sets instead of > orig_. As the latter is also set by osnoise_put_(), plus long > long options set in both the old and new implementation, the old > behavior was likely a mistake, and should not matter for now, as the > options are only restored once at the end of tracing and neither > nor orig_ field is read again. >=20 > Assisted-by: Claude:claude-opus-4-6 > Signed-off-by: Tomas Glozar > --- > tools/tracing/rtla/src/common.h | 79 +-- > tools/tracing/rtla/src/osnoise.c | 836 ++++++------------------------- > tools/tracing/rtla/src/osnoise.h | 22 - > 3 files changed, 188 insertions(+), 749 deletions(-) While we're at it, can we move this code to common.c, and drop "osnoise" from the names, to move closer to using that only for the actual osnoise mode? Or if we really want to namespace things that are specific to the osnoise subsystem (i.e. everything implemented in trace_osnoise.c) but not specific with respect to the osnoise/timerlat split, I'd suggest something different like "osn_". > + * Long long option get/set/restore/put functions, generated from OSNOIS= E_LL_OPTIONS. > + */ > +#define OSNOISE_LL_OPTION(name, path, init_val) \ > +static long long \ > +osnoise_get_##name(struct osnoise_context *context) \ > +{ \ > + long long name; \ > + \ > + if (context->name !=3D (init_val)) \ > + return context->name; \ > + \ > + if (context->orig_##name !=3D (init_val)) \ > + return context->orig_##name; \ > + \ > + name =3D osnoise_read_ll_config(path); \ > + if (name < 0) \ > + return (init_val); \ > + \ > + context->orig_##name =3D name; \ > + return name; \ > +} \ > + \ > +int osnoise_set_##name(struct osnoise_context *context, long long name) = \ > +{ \ > + long long curr =3D osnoise_get_##name(context); \ > + int retval; \ > + \ > + if (curr =3D=3D (init_val)) \ > + return -1; \ > + \ > + retval =3D osnoise_write_ll_config(path, name); \ > + if (retval < 0) \ > + return -2; \ > + \ > + context->name =3D name; \ > + return 0; \ > +} \ Using "name" for the value is confusing... "val" would be better. > + \ > +void osnoise_restore_##name(struct osnoise_context *context) \ > +{ \ > + int retval; \ > + \ > + if (context->orig_##name =3D=3D (init_val)) \ > + return; \ > + \ > + if (context->orig_##name =3D=3D context->name) \ > + goto out_done_##name; \ > + \ > + retval =3D osnoise_write_ll_config(path, context->orig_##name); \ > + if (retval < 0) \ > + err_msg("Could not restore original " #name "\n"); \ > + \ > +out_done_##name: \ > + context->name =3D (init_val); \ > +} \ Why does the label need to have ##name in it? > + \ > +static void osnoise_put_##name(struct osnoise_context *context) \ > +{ \ > + osnoise_restore_##name(context); \ > + \ > + if (context->orig_##name =3D=3D (init_val)) \ > + return; \ > + \ > + context->orig_##name =3D (init_val); \ > +} [snip] > +/* > + * Flag option get/set/restore/put functions, generated from OSNOISE_FLA= G_OPTIONS. > + */ > +#define OSNOISE_FLAG_OPTION(name, option_str) \ > +static int osnoise_get_##name(struct osnoise_context *context) \ > +{ \ > + if (context->opt_##name !=3D OSNOISE_OPTION_INIT_VAL) \ > + return context->opt_##name; \ > + \ > + if (context->orig_opt_##name !=3D OSNOISE_OPTION_INIT_VAL) \ > + return context->orig_opt_##name; \ > + \ > + context->orig_opt_##name =3D osnoise_options_get_option(option_str); \ > + return context->orig_opt_##name; \ > +} \ > + \ > +int osnoise_set_##name(struct osnoise_context *context, bool onoff) \ > +{ \ > + int val =3D osnoise_get_##name(context); \ > + int retval; \ > + \ > + if (val =3D=3D OSNOISE_OPTION_INIT_VAL) \ > + return -1; \ > + \ > + if (val =3D=3D onoff) \ > + return 0; \ > + \ > + retval =3D osnoise_options_set_option(option_str, onoff); \ > + if (retval < 0) \ > + return -2; \ > + \ > + context->opt_##name =3D onoff; \ > + return 0; \ > +} \ > + \ > +void osnoise_restore_##name(struct osnoise_context *context) \ > +{ \ > + int retval; \ > + \ > + if (context->orig_opt_##name =3D=3D OSNOISE_OPTION_INIT_VAL) \ > + return; \ > + \ > + if (context->orig_opt_##name =3D=3D context->opt_##name) \ > + goto out_done_##name; \ > + \ > + retval =3D osnoise_options_set_option(option_str, context->orig_opt_##n= ame); \ > + if (retval < 0) \ > + err_msg("Could not restore original " option_str " option\n"); \ > + \ > +out_done_##name: \ > + context->opt_##name =3D OSNOISE_OPTION_INIT_VAL; \ > +} \ > + \ > +static void osnoise_put_##name(struct osnoise_context *context) \ > +{ \ > + osnoise_restore_##name(context); \ > + \ > + if (context->orig_opt_##name =3D=3D OSNOISE_OPTION_INIT_VAL) \ > + return; \ > + \ > + context->orig_opt_##name =3D OSNOISE_OPTION_INIT_VAL; \ > +} Can we reduce the amount of code we put in macros by moving some of the logic to osnoise_read/write_ll_config() and osnoise_get/set_optino()?=20 Or a non-macro wrapper around them if there are other callers that need the current behavior. Something like (assuming universal -1 invalid): static int osn_read_ll_config(const char *rel_path, long long *val, long lo= ng *orig) static int osn_write_ll_config(const char *rel_path, long long *val, long l= ong *orig) static int osn_get_option(const char *name, int *val, int *orig) static int osn_set_option(const char *name, int *val, int *orig) -Crystal (who wishes we were using a modern language that didn't require al= l this macro stuff)