Re: [RFC PATCH 2/2] bpf: let bpf_warn_invalid_xdp_action() report more info

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Paolo Abeni <pabeni@redhat.com> writes:

> Hello,
>
> On Wed, 2021-11-17 at 16:48 -0800, Alexei Starovoitov wrote:
>> On Mon, Nov 15, 2021 at 06:09:28PM +0100, Toke Høiland-Jørgensen wrote:
>> > Paolo Abeni <pabeni@redhat.com> writes:
>> > 
>> > > > > -	pr_warn_once("%s XDP return value %u, expect packet loss!\n",
>> > > > > +	pr_warn_once("%s XDP return value %u on prog %d dev %s attach type %d, expect packet loss!\n",
>> > > > >  		     act > act_max ? "Illegal" : "Driver unsupported",
>> > > > > -		     act);
>> > > > > +		     act, prog->aux->id, dev->name, prog->expected_attach_type);
>> > > > 
>> > > > This will only ever trigger once per reboot even if the message differs,
>> > > > right? Which makes it less useful as a debugging aid; so I'm not sure if
>> > > > it's really worth it with this intrusive change unless we also do
>> > > > something to add a proper debugging aid (like a tracepoint)...
>> > > 
>> > > Yes, the idea would be to add a tracepoint there, if there is general
>> > > agreement about this change.
>> > > 
>> > > I think this patch is needed because the WARN_ONCE splat gives
>> > > implicitly information about the related driver and attach type.
>> > > Replacing with a simple printk we lose them.
>> > 
>> > Ah, right, good point. Pointing that out in the commit message might be
>> > a good idea; otherwise people may miss that ;)
>> 
>> Though it's quite a churn across the drivers I think extra verbosity here is justified.
>> I'd only suggest to print stable things. Like prog->aux->id probably has
>> little value for the person looking at the logs. That prog id is likely gone.
>> If it was prog->aux->name it would be more helpful.
>> Same with expected_attach_type. Why print it at all?
>> tracepoint is probably good idea too.
>
> Thanks for the feedback.
>
> I tried to select the additional arguments to allow the user/admin
> tracking down which program is causing the issue and why. I'm a
> complete newbe wrt XDP, so likely my choice were naive.
>
> I thought the id identifies the program in an unambiguous manner. I
> understand the program could be unloaded meanwhile, but if that is not
> the case the id should be quite useful. Perhaps we could dump both the
> id and the name?
>
> I included the attach type as different types support/allow different
> actions: the same program could cause the warning or not depending on
> it. If that is not useful I can drop the attach type from the next
> iteration.

The attach type identifies DEVMAP and CPUMAP programs, but just printing
it as a number probably doesn't make sense. So maybe something like:

switch(prog->expected_attach_type) {
    case BPF_XDP_DEVMAP:
    case BPF_XDP_CPUMAP:
      pr_warn_once("Illegal XDP return value %u from prog %s(%d) in %s!\n",
                   act, prog->aux_name, prog->aux->id,
                   prog->expected_attach_type == BPF_XDP_DEVMAP ? "devmap" : "cpumap");
      break;
    default:
      pr_warn_once("%s XDP return value %u on prog %s(%d) dev %s, expect packet loss!\n",
                   act > act_max ? "Illegal" : "Driver unsupported",
                   act, prog->aux->name, prog->aux->id, dev->name);
      break;
}      

-Toke