Re: [PATCH bpf-next v1 05/13] bpf: net_sched: Support implementation of Qdisc_ops in bpf

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/13/24 3:29 PM, Amery Hung wrote:
> +static int bpf_qdisc_init_member(const struct btf_type *t,
> +				 const struct btf_member *member,
> +				 void *kdata, const void *udata)
> +{
> +	const struct Qdisc_ops *uqdisc_ops;
> +	struct Qdisc_ops *qdisc_ops;
> +	u32 moff;
> +
> +	uqdisc_ops = (const struct Qdisc_ops *)udata;
> +	qdisc_ops = (struct Qdisc_ops *)kdata;
> +
> +	moff = __btf_member_bit_offset(t, member) / 8;
> +	switch (moff) {
> +	case offsetof(struct Qdisc_ops, priv_size):
> +		if (uqdisc_ops->priv_size)

bpf_struct_ops_map_update_elem() has enforced non function pointer member must 
be zero if ->init_member() returns 0, so this check is unnecessary.

> +			return -EINVAL;
> +		return 1;
> +	case offsetof(struct Qdisc_ops, static_flags):
> +		if (uqdisc_ops->static_flags)

Same here.

case priv_size and static_flags should be not needed, just return 0.

> +			return -EINVAL;
> +		return 1;
> +	case offsetof(struct Qdisc_ops, peek):
> +		if (!uqdisc_ops->peek)

bpf_struct_ops_map_update_elem() will assign the trampoline (that will call the 
bpf prog) to qdisc_ops->peek if the "u"qdisc_ops->peek has the prog fd.

This test is not necessary also.

> +			qdisc_ops->peek = qdisc_peek_dequeued;

Always do this assignment



> +		return 1;

and return 0 here. Allow the bpf_struct_ops_map_update_elem() to do the needed 
fd testing instead and reassign the qdisc_ops->peek with the trampoline if needed.

> +	case offsetof(struct Qdisc_ops, id):
> +		if (bpf_obj_name_cpy(qdisc_ops->id, uqdisc_ops->id,
> +				     sizeof(qdisc_ops->id)) <= 0)
> +			return -EINVAL;
> +		return 1;
> +	}
> +
> +	return 0;
> +}