Message-ID: <a8dc7739-2227-eebd-1ba5-6b56d6188888@fb.com>
Date:   Mon, 18 May 2020 23:21:10 -0700
From:   Yonghong Song <yhs@...com>
To:     Alan Maguire <alan.maguire@...cle.com>
CC:     <ast@...nel.org>, <daniel@...earbox.net>, <bpf@...r.kernel.org>,
        <joe@...ches.com>, <linux@...musvillemoes.dk>,
        <arnaldo.melo@...il.com>, <kafai@...com>, <songliubraving@...com>,
        <andriin@...com>, <john.fastabend@...il.com>,
        <kpsingh@...omium.org>, <linux-kernel@...r.kernel.org>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 bpf-next 2/7] bpf: move to generic BTF show support,
 apply it to seq files/strings



On 5/18/20 2:46 AM, Alan Maguire wrote:
> On Wed, 13 May 2020, Yonghong Song wrote:
> 
>>
>>> +struct btf_show {
>>> +	u64 flags;
>>> +	void *target;	/* target of show operation (seq file, buffer) */
>>> +	void (*showfn)(struct btf_show *show, const char *fmt, ...);
>>> +	const struct btf *btf;
>>> +	/* below are used during iteration */
>>> +	struct {
>>> +		u8 depth;
>>> +		u8 depth_shown;
>>> +		u8 depth_check;
>>
>> I have some difficulties to understand the relationship between
>> the above three variables. Could you add some comments here?
>>
> 
> Will do; sorry the code got a bit confusing. The goal is to track
> which sub-components in a data structure we need to display.  The
> "depth" variable tracks where we are currently; "depth_shown"
> is the depth at which we have something nonzer to display (perhaps
> "depth_to_show" would be a better name?). "depth_check" tells

"depth_to_show" is indeed better.

> us whether we are currently checking depth or doing printing.
> If we're checking, we don't actually print anything, we merely note
> if we hit a non-zero value, and if so, we set "depth_shown"
> to the depth at which we hit that value.
> 
> When we show a struct, union or array, we will only display an
> object has one or more non-zero members.  But because
> the struct can in turn nest a struct or array etc, we need
> to recurse into the object.  When we are doing that, depth_check
> is set, and this tells us not to do any actual display. When
> that recursion is complete, we check if "depth_shown" (depth
> to show) is > depth (i.e. we found something) and if it is
> we go on to display the object (setting depth_check to 0).

Thanks for the explanation. Putting them in the comments
will be great.

> 
> There may be a better way to solve this problem of course,
> but I wanted to avoid storing values where possible as
> deeply-nested data structures might overrun such storage.
> 
[...]
>>> +
>>> +	/*
>>> +	 * The goal here is to build up the right number of pointer and
>>> +	 * array suffixes while ensuring the type name for a typedef
>>> +	 * is represented.  Along the way we accumulate a list of
>>> +	 * BTF kinds we have encountered, since these will inform later
>>> +	 * display; for example, pointer types will not require an
>>> +	 * opening "{" for struct, we will just display the pointer value.
>>> +	 *
>>> +	 * We also want to accumulate the right number of pointer or array
>>> +	 * indices in the format string while iterating until we get to
>>> +	 * the typedef/pointee/array member target type.
>>> +	 *
>>> +	 * We start by pointing at the end of pointer and array suffix
>>> +	 * strings; as we accumulate pointers and arrays we move the pointer
>>> +	 * or array string backwards so it will show the expected number of
>>> +	 * '*' or '[]' for the type.  BTF_SHOW_MAX_ITER of nesting of pointers
>>> +	 * and/or arrays and typedefs are supported as a precaution.
>>> +	 *
>>> +	 * We also want to get typedef name while proceeding to resolve
>>> +	 * type it points to so that we can add parentheses if it is a
>>> +	 * "typedef struct" etc.
>>> +	 */
>>> +	for (i = 0; i < BTF_SHOW_MAX_ITER; i++) {
>>> +
>>> +		switch (BTF_INFO_KIND(t->info)) {
>>> +		case BTF_KIND_TYPEDEF:
>>> +			if (!type_name)
>>> +				type_name = btf_name_by_offset(show->btf,
>>> +							       t->name_off);
type_name should never be NULL for valid vmlinux BTF.

>>> +			kinds |= BTF_KIND_BIT(BTF_KIND_TYPEDEF);
>>> +			id = t->type;
>>> +			break;
>>> +		case BTF_KIND_ARRAY:
>>> +			kinds |= BTF_KIND_BIT(BTF_KIND_ARRAY);
>>> +			parens = "[";
>>> +			array = btf_type_array(t);
>>> +			if (!array)
array will never be NULL here.
>>> +				return show->state.type_name;
>>> +			if (!t)
t will never be NULL here.
>>> +				return show->state.type_name;
>>> +			if (array_suffix > array_suffixes)
>>> +				array_suffix -= 2;
>>> +			id = array->type;
>>> +			break;
>>> +		case BTF_KIND_PTR:
>>> +			kinds |= BTF_KIND_BIT(BTF_KIND_PTR);
>>> +			if (ptr_suffix > ptr_suffixes)
>>> +				ptr_suffix -= 1;
>>> +			id = t->type;
>>> +			break;
>>> +		default:
>>> +			id = 0;
>>> +			break;
>>> +		}
>>> +		if (!id)
>>> +			break;
>>> +		t = btf_type_skip_qualifiers(show->btf, id);
t should never be NULL here.
>>> +		if (!t)
>>> +			return show->state.type_name;
>>> +	}
>>
>> Do we do pointer tracing here? For example
>> struct t {
>> 	int *m[5];
>> }
>>
>> When trying to access memory, the above code may go through
>> ptr->array and out of loop when hitting array element type "int"?
>>
> 
> I'm not totally sure I understand the question so I'll
> try and describe how the above is supposed to work. I
> think there's a bug here alright.
> 
> In the above case, when we reach the "m" field of "struct t",
> the code  should start with the BTF_KIND_ARRAY, set up
> the array suffix, then get the array type which is a PTR
> and we will set up the ptr suffix to be "*" and we set
> the id to the id associated with "int", and
> btf_type_skip_qualifiers() will use that id to look up
> the new value for the type used in btf_name_by_offset().
> So on the next iteration we hit the int itself and bail from
> the loop, having noted that we've got a _PTR and _ARRAY set in
> the "kinds" bitfield.
> 
> Then we look up the int type using "t" with btf_name_by_offset,
> so we end up displaying "(int *m[])" as the type.

Thanks for explanation. Previously I thought this somehow
may be related to tracing data. Looks it is only for
*constructing* type names. So it largely looks fine though.

>    
> However the code assumes we don't need the parentheses for
> the array if we have encountered a pointer; that's never
> the case.  We only should eliminate the opening parens
> for a struct or union "{" in such cases, as in those cases
> we have a pointer to the struct rather than a nested struct.
> So that needs to be fixed. Are the other problems here you're
> seeing that the above doesn't cover?

A few minor comments in the above.

> 
>>> +	/* We may not be able to represent this type; bail to be safe */
>>> +	if (i == BTF_SHOW_MAX_ITER)
>>> +		return show->state.type_name;
>>> +
>>> +	if (!type_name)
>>> +		type_name = btf_name_by_offset(show->btf, t->name_off);
>>> +
>>> +	switch (BTF_INFO_KIND(t->info)) {
>>> +	case BTF_KIND_STRUCT:
>>> +	case BTF_KIND_UNION:
>>> +		prefix = BTF_INFO_KIND(t->info) == BTF_KIND_STRUCT ?
>>> +			 "struct" : "union";
>>> +		/* if it's an array of struct/union, parens is already set */
>>> +		if (!(kinds & (BTF_KIND_BIT(BTF_KIND_ARRAY))))
>>> +			parens = "{";
>>> +		break;
>>> +	case BTF_KIND_ENUM:
>>> +		prefix = "enum";
>>> +		break;
>>> +	default:
>>> +		allow_anon = false;
[...]
>>> +	if (elem_type && btf_type_is_int(elem_type)) {
>>> +		u32 int_type = btf_type_int(elem_type);
>>> +
>>> +		encoding = BTF_INT_ENCODING(int_type);
>>> +
>>> +		/*
>>> +		 * BTF_INT_CHAR encoding never seems to be set for
>>> +		 * char arrays, so if size is 1 and element is
>>> +		 * printable as a char, we'll do that.
>>> +		 */
>>> +		if (elem_size == 1) > +			encoding =
>>> BTF_INT_CHAR;
>>
>> Some char array may be printable and some may not be printable,
>> how did you differentiate this?
>>
> 
> I should probably change the logic to ensure all chars
> (before a \0) are printable. I'll do that for v2. We will always
> have cases (e.g. the skb cb[] field) where the char[] is not
> intended as a string, but I think the utility of showing them as
> strings where possible is worthwhile.

Make sense. Thanks!

> 
> Thanks again for reviewing!
> 
> Alan
> 

Powered by blists - more mailing lists