On Tue, Jun 23, 2020 at 01:07:07PM +0100, Alan Maguire wrote:

printk supports multiple pointer object type specifiers (printing
netdev features etc).  Extend this support using BTF to cover
arbitrary types.

Is there any plans to cover (all?) existing %p extensions?

"%pT"

One letter namespace is quite busy area. Perhaps %pOT ?

specifies the typed format, and the pointer
argument is a "struct btf_ptr *" where struct btf_ptr is as follows:

struct btf_ptr {
        void *ptr;
        const char *type;
        u32 id;
};

Either the "type" string ("struct sk_buff") or the BTF "id" can be
used to identify the type to use in displaying the associated "ptr"
value.  A convenience function to create and point at the struct
is provided:

        printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));

When invoked, BTF information is used to traverse the sk_buff *
and display it.  Support is present for structs, unions, enums,
typedefs and core types (though in the latter case there's not
much value in using this feature of course).

Default output is indented, but compact output can be specified
via the 'c' option.  Type names/member values can be suppressed
using the 'N' option.  Zero values are not displayed by default
but can be using the '0' option.  Pointer values are obfuscated
unless the 'x' option is specified.  As an example:

  struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
  pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));

...gives us:

(struct sk_buff){
 .transport_header = (__u16)65535,
 .mac_header = (__u16)65535,
 .end = (sk_buff_data_t)192,
 .head = (unsigned char *)0x000000006b71155a,
 .data = (unsigned char *)0x000000006b71155a,
 .truesize = (unsigned int)768,
 .users = (refcount_t){
  .refs = (atomic_t){
   .counter = (int)1,
  },
 },
 .extensions = (struct skb_ext *)0x00000000f486a130,
}

I don't see how it looks on a real console when kernel dumps something.
Care to provide? These examples better to have documented.

printk output is truncated at 1024 bytes.  For cases where overflow
is likely, the compact/no type names display modes may be used.

How * is handled? (I mean %*pOT case)

...

+#define	BTF_PTR_TYPE(ptrval, typeval) \
+	(&((struct btf_ptr){.ptr = ptrval, .type = #typeval}))
+
+#define BTF_PTR_ID(ptrval, idval) \
+	(&((struct btf_ptr){.ptr = ptrval, .id = idval}))

Wouldn't be better if these will leave in its own (linker) section?

...

+static noinline_for_stack
+char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
+		 const char *fmt)
+{

+	struct btf_ptr *bp = (struct btf_ptr *)ptr;

Unneeded casting.

+	u8 btf_kind = BTF_KIND_TYPEDEF;
+	const struct btf_type *t;
+	const struct btf *btf;
+	char *buf_start = buf;
+	const char *btf_type;
+	u64 flags = 0, mod;
+	s32 btf_id;
+
+	if (check_pointer(&buf, end, ptr, spec))
+		return buf;
+
+	if (check_pointer(&buf, end, bp->ptr, spec))
+		return buf;

+	while (isalnum(*fmt)) {
+		mod = btf_modifier_flag(*fmt);
+		if (!mod)
+			break;
+		flags |= mod;
+		fmt++;
+	}

Can't we have explicitly all handled flags here, like other extensions do?

+	btf = bpf_get_btf_vmlinux();
+	if (IS_ERR_OR_NULL(btf))
+		return ptr_to_id(buf, end, bp->ptr, spec);
+
+	if (bp->type != NULL) {
+		btf_type = bp->type;
+
+		if (strncmp(bp->type, "struct ", strlen("struct ")) == 0) {
+			btf_kind = BTF_KIND_STRUCT;
+			btf_type += strlen("struct ");
+		} else if (strncmp(btf_type, "union ", strlen("union ")) == 0) {
+			btf_kind = BTF_KIND_UNION;
+			btf_type += strlen("union ");
+		} else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
+			btf_kind = BTF_KIND_ENUM;
+			btf_type += strlen("enum ");
+		}

Can't you provide a simple structure and do this in a loop?
Or even something like match_[partial]string() to implement?

+		if (strlen(btf_type) == 0)

Interesting way of checking btf_type == '\0'.

+			return ptr_to_id(buf, end, bp->ptr, spec);
+
+		/*
+		 * Assume type specified is a typedef as there's not much
+		 * benefit in specifying int types other than wasting time
+		 * on BTF lookups; we optimize for the most useful path.
+		 *
+		 * Fall back to BTF_KIND_INT if this fails.
+		 */
+		btf_id = btf_find_by_name_kind(btf, btf_type, btf_kind);
+		if (btf_id < 0)
+			btf_id = btf_find_by_name_kind(btf, btf_type,
+						       BTF_KIND_INT);
+	} else if (bp->id > 0)
+		btf_id = bp->id;
+	else
+		return ptr_to_id(buf, end, bp->ptr, spec);
+

+	if (btf_id > 0)
+		t = btf_type_by_id(btf, btf_id);
+	if (btf_id <= 0 || !t)
+		return ptr_to_id(buf, end, bp->ptr, spec);

This can be easily incorporated in previous conditional tree.

+	buf += btf_type_snprintf_show(btf, btf_id, bp->ptr, buf,
+				      end - buf_start, flags);
+
+	return widen_string(buf, buf - buf_start, end, spec);
+}

-- 
With Best Regards,
Andy Shevchenko