Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Steven Rostedt <rostedt@goodmis.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
CC:     LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [GIT PULL] tracing: histogram fix and take 2 on the
 __string_len() marcros
Thread-Topic: [GIT PULL] tracing: histogram fix and take 2 on the
 __string_len() marcros
Thread-Index: AQHXeeYCjeFzOpspFUa2lky1xLSwtatF6D4AgAAHHICAAAJ6gIAAKoGAgAAzoICAAAkHAIABC1OA
Date:   Sat, 17 Jul 2021 16:51:58 +0000
Message-ID: <F5824BF9-1F6F-4882-A6D9-0943DCFF55DA@oracle.com>
References: <20210715215753.4a314e97@rorschach.local.home>
 <CAHk-=wiWdG6jqKhdU62b06-DtESVxHVK8MA23iV+6fB5hnGEAw@mail.gmail.com>
 <20210716143705.56001390@oasis.local.home>
 <CAHk-=wjWosrcv2=6m-=YgXRKev=5cnCg-1EhqDpbRXT5z6eQmg@mail.gmail.com>
 <20210716171805.55aed9de@oasis.local.home>
 <26A9CF5A-0724-4D4B-BC59-09849EC33326@oracle.com>
 <20210716205510.7f565b98@oasis.local.home>
In-Reply-To: <20210716205510.7f565b98@oasis.local.home>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SNrwv//huDQ9YgM+VA12VZFnUI1ox5F+rtkjWRnnxQHYdOy2BNp5FwTQPDoe?=
 =?us-ascii?Q?4IYbeA02LBUOA9zzD/oAv2YXx49X8lvm2geZqPHGyrrYWbuc1WescUwn0njA?=
 =?us-ascii?Q?XRF9J/XOekiA6CmrZYzsKvM+dK86eaj8yllNzjGQ2edtFrx6UvwJ4Il4h1bZ?=
 =?us-ascii?Q?UAbzmp3PdBS1dpvzaScLqNGxCMUHLCIZXz2FERW/CM4v6EYhB5aRJceiJ7ER?=
 =?us-ascii?Q?BRuJSPUC9X5lP9u3roNCYWwMA+QH819P+GNB9+ExxUYyUHUwD+edsUXY1jy1?=
 =?us-ascii?Q?3smEJpLSsrfl/pOW4Hhkip02RkvFagzloy7Q5o1enCRuqbmqnLKN0CArBTCp?=
 =?us-ascii?Q?GOa0bPpaEIROBApkBk4cXnU2yUXSuuChF0bLwZCyi+90RaMY37QahJ2JgEU7?=
 =?us-ascii?Q?TwP88LxRYB7StHnks0I9aI5XOziNvEa0NiZbPSikfyCqo9c/cRfRaKudjPyb?=
 =?us-ascii?Q?62pzCny4J5+i0uBzoIYV31u+5VkM7IYGkNqL/lx6jT9AZPCN26u9AjkZlFnx?=
 =?us-ascii?Q?C4mtl45Oa4R+udlz91MycK8OUD8RPcYXz2QN/8Qyez7FykXePaHAv4db/UOl?=
 =?us-ascii?Q?5YXyH+jsmGYsFn+wxr34GVbGGJN+yLnTy5k/b5M6WncHPq+tqZTwfbx9O5Xm?=
 =?us-ascii?Q?sZFQRu2q/n6YRQsu4cAhySuOlYEQ3XQOG8CPu6xEOjmzGH4iAEP/+hAvuCia?=
 =?us-ascii?Q?Z33HBE0GrNsRhP3Cg8T/KOL+1sYZSpLTa6Y2jvNQof8QmTngluIqbnTLryEJ?=
 =?us-ascii?Q?FFLKtEsT1/EAPlG5wjSMsfs7wZOVuCzzW/G8k1XtKsAZntclbci27/bMZmES?=
 =?us-ascii?Q?DJfJjrVbUMT5arzysva/N7+dOpIPtRnVNBjgPfZj4SveGBUTN69l4v+aKLaN?=
 =?us-ascii?Q?e/L5TtdIY3pqFyQeQRkqLe0JYxt5ivUz6g7q7qUf02kWf6ZgrmlozYhll/L8?=
 =?us-ascii?Q?DDDQhOO0H91N8RxND4Znky/lIhUn2GQvgWQDfhukLNpQZXNHVNskK3+GJSAt?=
 =?us-ascii?Q?d7Xfr8ZHwuNQABAsIEqIVaOuelHk1COCUMmgsMJfQBgzGPuVuBhz7Po2feeF?=
 =?us-ascii?Q?aqZywSinUctiXi/KJNzSRgGVt+fYkXurV/QCipHK6QVsUO3dh0vQMuXddM6y?=
 =?us-ascii?Q?S7ItDsyC16EMXsi3XuTSHgR2e3/CSaNbt0A9z+dW1VhSGYDtqv13Q2Xf71mp?=
 =?us-ascii?Q?jHLuBOFFFRzgd564B41fqC7i91h2c28gxbJWNRa+aQXdwICHTyxfha9Asg7S?=
 =?us-ascii?Q?WeNMvoZfJ8kLHeb5VvwNAbaN1wEytQLAivlwPuq3i2J8nwYhB32tb97f+i8O?=
 =?us-ascii?Q?N6UdRIAJ6WqrzrDkplnYp8+0?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F8A9E6AA9EE5A346BCFFB745E4BE0755@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4688.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ef15f503-4dca-4734-0a06-08d9494330f4
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jul 2021 16:51:58.1904
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: lzS0F+zC7EvraMeTyJoXap08mmxGi9cNWwR6ICEPPffn7cMVstDyOOilI6QYnkM+BxDioWgc+XwtY3W0v2umtw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4257
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=10048 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 malwarescore=0
 mlxlogscore=999 suspectscore=0 bulkscore=0 spamscore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2107170101
X-Proofpoint-ORIG-GUID: pNPbs3G_gBZmcopOnptECa6GxprQxGqH
X-Proofpoint-GUID: pNPbs3G_gBZmcopOnptECa6GxprQxGqH
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Jul 16, 2021, at 8:55 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>=20
> How about this. I'll just give you the patch and you can apply it to
> your tree. I updated it with documentation, and use memcpy instead of
> strncpy() as it is replacing memcpy() and strncpy() will cause people
> to question the code (as Linus has).
>=20
> Here's my latest patch. Feel free to apply it to your tree.

Thanks. I can carry whichever version of this patch that both you
and Linus are happy with. I will apply my changes to use __string_len()
on top of it, and send the whole mess when v5.15 opens in a couple
months.


> Hopefully
> it wont conflict with other work I'm doing. But if it does, we'll work
> it out. I don't have any code that relies on it.
>=20
> -- Steve
>=20
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Subject: [PATCH] tracing: Add trace_event helper macros __string_len() an=
d
> __assign_str_len()
>=20
> There's a few cases that a string that is to be recorded in a trace event=
,
> does not have a terminating 'nul' character, and instead, the tracepoint
> passes in the length of the string to record.
>=20
> Add two helper macros to the trace event code that lets this work easier,
> than tricks with "%.*s" logic.
>=20
>  __string_len() which is similar to __string() for declaration, but takes=
 a
>                 length argument.
>=20
>  __assign_str_len() which is similar to __assign_str() for assiging the
>                 string, but it too takes a length argument.
>=20
> Note, the TRACE_EVENT() macro will allocate the location on the ring
> buffer to 'len + 1', that will be used to store the string into. It is a
> requirement that the 'len' used for this is a most the length of the
> string being recorded.
>=20
> This string can still use __get_str() just like strings created with
> __string() can use to retrieve the string.
>=20
> Link: https://lore.kernel.org/linux-nfs/20210513105018.7539996a@gandalf.l=
ocal.home/
>=20
> Tested-by: Chuck Lever <chuck.lever@oracle.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> include/trace/trace_events.h               | 22 ++++++++++++++++++
> samples/trace_events/trace-events-sample.h | 27 ++++++++++++++++++++++
> 2 files changed, 49 insertions(+)
>=20
> diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
> index acc17194c160..08810a463880 100644
> --- a/include/trace/trace_events.h
> +++ b/include/trace/trace_events.h
> @@ -102,6 +102,9 @@ TRACE_MAKE_SYSTEM_STR();
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
>=20
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(char, item, -1)
>=20
> @@ -197,6 +200,9 @@ TRACE_MAKE_SYSTEM_STR();
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
>=20
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
>=20
> @@ -459,6 +465,9 @@ static struct trace_event_functions trace_event_type_=
funcs_##call =3D {	\
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
>=20
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
>=20
> @@ -507,6 +516,9 @@ static struct trace_event_fields trace_event_fields_#=
#call[] =3D {	\
> #define __string(item, src) __dynamic_array(char, item,			\
> 		    strlen((src) ? (const char *)(src) : "(null)") + 1)
>=20
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, (len) +=
 1)
> +
> /*
>  * __bitmask_size_in_bytes_raw is the number of bytes needed to hold
>  * num_possible_cpus().
> @@ -670,10 +682,20 @@ static inline notrace int trace_event_get_offsets_#=
#call(		\
> #undef __string
> #define __string(item, src) __dynamic_array(char, item, -1)
>=20
> +#undef __string_len
> +#define __string_len(item, src, len) __dynamic_array(char, item, -1)
> +
> #undef __assign_str
> #define __assign_str(dst, src)						\
> 	strcpy(__get_str(dst), (src) ? (const char *)(src) : "(null)");
>=20
> +#undef __assign_str_len
> +#define __assign_str_len(dst, src, len)					\
> +	do {								\
> +		memcpy(__get_str(dst), (src), (len));			\
> +		__get_str(dst)[len] =3D '\0';				\
> +	} while(0)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
>=20
> diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_e=
vents/trace-events-sample.h
> index 13a35f7cbe66..e61471ab7d14 100644
> --- a/samples/trace_events/trace-events-sample.h
> +++ b/samples/trace_events/trace-events-sample.h
> @@ -141,6 +141,33 @@
>  *         In most cases, the __assign_str() macro will take the same
>  *         parameters as the __string() macro had to declare the string.
>  *
> + *   __string_len: This is a helper to a __dynamic_array, but it underst=
ands
> + *	   that the array has characters in it, and with the combined
> + *         use of __assign_str_len(), it will allocate 'len' + 1 bytes
> + *         in the ring buffer and add a '\0' to the string. This is
> + *         useful if the string being saved has no terminating '\0' byte=
.
> + *         It requires that the length of the string is known as it acts
> + *         like a memcpy().
> + *
> + *         Declared with:
> + *
> + *         __string_len(foo, bar, len)
> + *
> + *         To assign this string, use the helper macro __assign_str_len(=
).
> + *
> + *         __assign_str(foo, bar, len);
> + *
> + *         Then len + 1 is allocated to the ring buffer, and a nul termi=
nating
> + *         byte is added. This is similar to:
> + *
> + *         memcpy(__get_str(foo), bar, len);
> + *         __get_str(foo)[len] =3D 0;
> + *
> + *        The advantage of using this over __dynamic_array, is that it
> + *        takes care of allocating the extra byte on the ring buffer
> + *        for the '\0' terminating byte, and __get_str(foo) can be used
> + *        in the TP_printk().
> + *
>  *   __bitmask: This is another kind of __dynamic_array, but it expects
>  *         an array of longs, and the number of bits to parse. It takes
>  *         two parameters (name, nr_bits), where name is the name of the
> --=20
> 2.31.1
>=20

--
Chuck Lever