| Message ID | 20220612085330.1451496-6-asmadeus@codewreck.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | None | expand |
On Sun, 12 Jun 2022 17:53:28 +0900 Dominique Martinet <asmadeus@codewreck.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@codewreck.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
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;
Signed-off-by: Dominique Martinet <asmadeus@codewreck.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? 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(-)