Re: [PATCH v2 10/20] kernel-shark: Start using data streams

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

On 5.11.20 г. 20:17 ч., Steven Rostedt wrote:
> On Thu, 5 Nov 2020 16:58:51 +0200
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> I see the problem. This is definitely wrong.
>>
>> What if in addition to "n_streams" I add another counter called
>> "last_stream_added" and initialize this counter to -1?
>>
>> Then I can add streams like this:
>>
>> 	kshark_ctx->stream[++kshark_ctx->last_stream_added] = stream;
>> 	++kshark_ctx->n_streams;
>>
>>> You may want to do instead:
> 
> I'm thinking of doing something like this:
> 
> struct kshark_ctx {
> 	[..]
> 	unsigned int free_stream;
> 	[..]
> };
> 
> 
> 	if (kshark_ctx->free_stream >= kshark_ctx->n_streams) {
> 		kshark_ctx->stream[++kshark_ctx->n_streams] = stream;
> 		kshark_ctx->free_stream = kshark_ctx->n_streams;
> 
> /* BTW, the stream array should be allocated to the n_streams, and
>     reallocated when it grows.  I don't think we want a huge stream array to
>     handle all the bits when not used. */
> 
> 	} else {
> 		int new_stream = kshark_ctx->free_stream;
> 
> 		kshark_ctx->free_stream = kshark_index(kshark_ctx->stream[new_stream]);
> 		kshark_ctx->stream[new_stream] = stream;
> 	}
> 
> 
> For freeing (index i):
> 
> 	kshark_ctx->stream[i] = kshark_ptr(kshark_ctx->free_stream);
> 	kshark_ctx->free_stream = i;
> 
> 
> We could define the following (note, I just used these names for the
> functions, they could be named something else):
> 
> 
> #define KSHARK_INDEX_MASK ((1 << NR_OF_BITS_FOR_STREAM) - 1)
> 
> #define KSHARK_INVALID_STREAM (~((1UL << NR_OF_BITS_FOR_STREAM) - 1))
> 
> static inline int kshark_index(void *ptr)
> {
> 	unsigned long index = (unsigned long)ptr;
> 
> 	return (int)(index & KSHARK_INDEX_MASK);
> }
> 
> static inline void *kshark_ptr(unsigned int index)
> {
> 	unsigned long p;
> 
> 	p = KSHARK_INVALID_STREAM | index;
> 
> 	return (void *)p;
> }
> 
> The KSHARK_INVALID_STREAM and KSHARK_INDEX_MASK, would allow us to do
> something like this if we wanted to loop through all streams:
> 
> static inline bool kshark_is_valid_stream(void *ptr)
> {
> 	unsigned long p = (unsigned long)ptr;
> 
> 	return (p & KSHARK_INVALID_STREAM) == KSHARK_INVALID_STREAM;
> }
> 
> The above works because the address of setting all those bits, would put
> the address into the kernel space (illegal user space address).
> 
> 
> 	for (i = 0; i < kshark_ctx->n_streams; i++) {
> 		if (!kshark_is_valid_stream(kshark_ctx->stream[i]))
> 			continue;
> 		/* process valid stream */
> 	}
>

Hi Steven,

I am not sure I understand correctly your pseudo-code, so please correct 
me if my interpretation is wrong.

In the normal case when a new stream is added the corresponding object 
will be allocated and added to the array of pointers. Later if a stream 
is removed, instead of freeing the memory we will just manipulate it 
pointer so that it point to nowhere and this manipulation can be 
detected by the kshark_is_valid_stream(). Now if we want to add stream 
again, we will take the broken pointer, will restore its original value 
and will reuse the object without a new allocations.

And at the very end we will have to free all pointers (original or 
manipulated).

Is this what you are suggesting?

Thanks!
Y.


> -- Steve
>