| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220612184659.6dff5107@rorschach.local.home>
Date: Sun, 12 Jun 2022 18:46:59 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Dominique Martinet <asmadeus@...ewreck.org>
Cc: Christian Schoenebeck <linux_oss@...debyte.com>,
Tyler Hicks <tyhicks@...ux.microsoft.com>,
Eric Van Hensbergen <ericvh@...il.com>,
Latchesar Ionkov <lucho@...kov.net>,
Ingo Molnar <mingo@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
v9fs-developer@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH 05/06] 9p fid refcount: add a 9p_fid_ref tracepoint
On Sun, 12 Jun 2022 17:53:28 +0900
Dominique Martinet <asmadeus@...ewreck.org> wrote:
This needs to have a change log. A tracepoint should never be added
without explaining why it is being added and its purpose.
> Signed-off-by: Dominique Martinet <asmadeus@...ewreck.org>
> ---
>
> This is the first time I add a tracepoint, so if anyone has time to
> check I didn't make something too stupid please have a look.
> I've mostly copied from netfs'.
>
> Also, a question if someone has time: I'd have liked to use the
> tracepoint in static inline wrappers for getref/putref, but it's not
> good to add the tracepoints to a .h, right?
Correct, because it can have unexpected side effects. Thus it is best
to call a wrapper function that is defined in a C file that will then
call the tracepoint. To avoid the overhead of the function call when
tracing is not enabled, you should use (in the header):
#include <linux/tracepoint-defs.h>
DECLARE_TRACEPOINT(<tracepoint-name>);
if (tracepoint_enabled(<tracepoint-name>))
do_<tracepoint-name>(...);
and in the C file have:
void do_<tracepoint-name>(...)
{
trace_<tracepoint-name>(...);
}
that calls the tracepoint. The tracepoint_enabled(<tracepoint-name>)()
is another special function that is created by the TRACE_EVENT() macro
to use, that is a static branch and not a real if statement. That is, it
is a nop that skips calling the wrapper function when not enabled, and
a jmp to call the wrapper function when the tracepoint is enabled.
How to do this is described in include/linux/tracepoint-defs.h and
there's an example of this use case in include/linux/mmap_lock.h.
> Especially with the CREATE_TRACE_POINTS macro...
> How do people usually go about that, just bite the bullet and make it
> a real function?
>
>
> include/trace/events/9p.h | 48 +++++++++++++++++++++++++++++++++++++++
> net/9p/client.c | 9 +++++++-
> 2 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/trace/events/9p.h b/include/trace/events/9p.h
> index 78c5608a1648..4dfa6d7f83ba 100644
> --- a/include/trace/events/9p.h
> +++ b/include/trace/events/9p.h
> @@ -77,6 +77,13 @@
> EM( P9_TWSTAT, "P9_TWSTAT" ) \
> EMe(P9_RWSTAT, "P9_RWSTAT" )
>
> +
> +#define P9_FID_REFTYPE \
> + EM( P9_FID_REF_CREATE, "create " ) \
> + EM( P9_FID_REF_GET, "get " ) \
> + EM( P9_FID_REF_PUT, "put " ) \
> + EMe(P9_FID_REF_DESTROY, "destroy" )
> +
> /* Define EM() to export the enums to userspace via TRACE_DEFINE_ENUM() */
> #undef EM
> #undef EMe
> @@ -84,6 +91,21 @@
> #define EMe(a, b) TRACE_DEFINE_ENUM(a);
>
> P9_MSG_T
> +P9_FID_REFTYPE
> +
> +/* And also use EM/EMe to define helper enums -- once */
> +#ifndef __9P_DECLARE_TRACE_ENUMS_ONLY_ONCE
> +#define __9P_DECLARE_TRACE_ENUMS_ONLY_ONCE
> +#undef EM
> +#undef EMe
> +#define EM(a, b) a,
> +#define EMe(a, b) a
> +
> +enum p9_fid_reftype {
> + P9_FID_REFTYPE
> +} __mode(byte);
> +
> +#endif
>
> /*
> * Now redefine the EM() and EMe() macros to map the enums to the strings
> @@ -96,6 +118,8 @@ P9_MSG_T
>
> #define show_9p_op(type) \
> __print_symbolic(type, P9_MSG_T)
> +#define show_9p_fid_reftype(type) \
> + __print_symbolic(type, P9_FID_REFTYPE)
>
> TRACE_EVENT(9p_client_req,
> TP_PROTO(struct p9_client *clnt, int8_t type, int tag),
> @@ -168,6 +192,30 @@ TRACE_EVENT(9p_protocol_dump,
> __entry->tag, 0, __entry->line, 16, __entry->line + 16)
> );
>
> +
> +TRACE_EVENT(9p_fid_ref,
> + TP_PROTO(struct p9_fid *fid, __u8 type),
> +
> + TP_ARGS(fid, type),
> +
> + TP_STRUCT__entry(
> + __field( int, fid )
> + __field( int, refcount )
> + __field( __u8, type )
> + ),
> +
> + TP_fast_assign(
> + __entry->fid = fid->fid;
> + __entry->refcount = refcount_read(&fid->count);
> + __entry->type = type;
> + ),
> +
> + TP_printk("%s fid %d, refcount %d",
> + show_9p_fid_reftype(__entry->type),
> + __entry->fid, __entry->refcount)
> +);
> +
> +
> #endif /* _TRACE_9P_H */
>
> /* This part must be outside protection */
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 5531b31e0fb2..fdeeda0a811d 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -907,8 +907,10 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt)
> GFP_NOWAIT);
> spin_unlock_irq(&clnt->lock);
> idr_preload_end();
> - if (!ret)
> + if (!ret) {
> + trace_9p_fid_ref(fid, P9_FID_REF_CREATE);
> return fid;
> + }
>
> kfree(fid);
> return NULL;
> @@ -920,6 +922,7 @@ static void p9_fid_destroy(struct p9_fid *fid)
> unsigned long flags;
>
> p9_debug(P9_DEBUG_FID, "fid %d\n", fid->fid);
> + trace_9p_fid_ref(fid, P9_FID_REF_DESTROY);
> clnt = fid->clnt;
> spin_lock_irqsave(&clnt->lock, flags);
> idr_remove(&clnt->fids, fid->fid);
> @@ -932,6 +935,8 @@ static void p9_fid_destroy(struct p9_fid *fid)
> * because trace_* functions can't be used there easily
> */
> struct p9_fid *p9_fid_get(struct p9_fid *fid) {
> + trace_9p_fid_ref(fid, P9_FID_REF_GET);
> +
> refcount_inc(&fid->count);
>
> return fid;
> @@ -941,6 +946,8 @@ int p9_fid_put(struct p9_fid *fid) {
> if (!fid || IS_ERR(fid))
> return 0;
>
> + trace_9p_fid_ref(fid, P9_FID_REF_PUT);
> +
> if (!refcount_dec_and_test(&fid->count))
> return 0;
>
Nothing stands out to me that would be wrong with the above.
-- Steve
Powered by blists - more mailing lists