Re: [PATCH v2 6/7] tracing/user_events: Use bits vs bytes for enabled status page data

[Beau Belgrave]

On Tue, Jul 26, 2022 at 08:14:12PM -0400, Steven Rostedt wrote:
> On Tue, 26 Jul 2022 17:02:49 -0700
> Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > > >  /* Limit how long of an event name plus args within the subsystem. */
> > > >  #define MAX_EVENT_DESC 512
> > > >  #define EVENT_NAME(user_event) ((user_event)->tracepoint.name)
> > > >  #define MAX_FIELD_ARRAY_SIZE 1024
> > > >  
> > > > +#define STATUS_BYTE(bit) ((bit) >> 3)
> > > > +#define STATUS_MASK(bit) (1 << ((bit) & 7))
> > > > +
> > > > +/* Internal bits to keep track of connected probes */
> > > > +#define EVENT_STATUS_FTRACE (1 << 0)
> > > > +#define EVENT_STATUS_PERF (1 << 1)
> > > > +#define EVENT_STATUS_OTHER (1 << 7)  
> > > 
> > > Did you mean to shift STATUS_OTHER by 7?
> > >   
> > 
> > Yes, it should be the value 128.
> > 
> > > Is EVENT_STATUS_OTHER suppose to be one of the flags within the 3 bits of
> > > the 7 in STATUS_MASK?
> > >   
> > 
> > My thought was that STATUS_OTHER would stay on the highest bit.
> > Then when we have other systems they would slot into (1 << 2), etc.
> > 
> > This may not be as important now since the byte is never given back to
> > the user and is only used when printing out status via the
> > user_events_status file in text form.
> 
> So, it is confusing because of STATUS_MASK() is bits 0,1,2 and we are
> only using bits 0 and 1, with a OTHER bit at bit 7. And it would be
> good to use the BIT() macro.
> 

Ah, I see the confusion. Sorry.  

EVENT_STATUS_* are internal bits that aren't used with STATUS_MASK or
STATUS_BYTE. It's only used to set and check the user event status byte
for checking if anything is attached and outputting which probe is
connected within the kernel side.

STATUS_BYTE and STATUS_MASK take a bit in a bitmap and figure out which
byte in the status mapping should be used and which bit in that byte
should be set/reset (mask) when it's enabled/disabled via a probe. Both
the user and kernel need to align on this logic.

IE: Bits above the lower 3 of the index/bit of the event to enable is the byte
and the lower 3 bits (& 7) is the actual bit to set.

For example if the user_event with the index 1024 is enabled, we need to
figure out which byte and bit represents that event when a probe is
attached.

I got into detail of this in the documentation for both a byte and long
wise checking of these values.

Hope that helps explain it.

> Is STATUS_OTHER suppose to be part of STATUS_MASK()?
> 
> -- Steve

Thanks,
-Beau