Re: [PATCH v17 11/16] fprobe: Rewrite fprobe on function-graph tracer

[Steven Rostedt]

On Wed, 16 Oct 2024 14:07:31 +0200
Sven Schnelle <svens@xxxxxxxxxxxxx> wrote:

> > +/* Return reserved data size in words */
> > +static inline int decode_fprobe_header(unsigned long val, struct fprobe **fp)
> > +{
> > +	unsigned long ptr;
> > +
> > +	ptr = (val & FPROBE_HEADER_PTR_MASK) | ~FPROBE_HEADER_PTR_MASK;
> > +	if (fp)
> > +		*fp = (struct fprobe *)ptr;
> > +	return val >> FPROBE_HEADER_PTR_BITS;
> > +}  
> 
> I think that still has the issue that the size is encoded in the
> leftmost fields of the pointer, which doesn't work on all
> architectures. I reported this already in v15
> (https://lore.kernel.org/all/yt9dmsjyx067.fsf@xxxxxxxxxxxxx/)

From what you said in v15:

> I haven't yet fully understood why this logic is needed, but the
> WARN_ON_ONCE triggers on s390. I'm assuming this fails because fp always
> has the upper bits of the address set on x86 (and likely others). As an
> example, in my test setup, fp is 0x8feec218 on s390, while it is
> 0xffff888100add118 in x86-kvm.

Since we only need to save 4 bits for size, we could have what it is
replacing always be zero or always be f, depending on the arch. The
question then is, is s390's 4 MSBs always zero?

Thus we could make it be:

static inline int decode_fprobe_header(unsigned long val, struct fprobe **fp)
{
	unsigned long ptr;

	ptr = (val & FPROBE_HEADER_PTR_MASK) | FPROBE_HEADER_MSB_MASK;
	if (fp)
		*fp = (struct fprobe *)ptr;
	return val >> FPROBE_HEADER_PTR_BITS;
}

And define FPROBE_HEADER_MSB_MASK to be either:

For most archs:

#define FPROBE_HEADER_MSB_MASK	(0xf << FPROBE_HEADER_PTR_BITS)

or on s390:

#define FPROBE_HEADER_MSB_MASK	(0x0)

Would this work?

-- Steve