On Wed, Feb 28, 2024 at 4:04 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Wed, 2024-02-28 at 15:55 -0800, Andrii Nakryiko wrote: > > On Tue, Feb 27, 2024 at 6:11 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > [...] > > > > Instead of adding struct_ops_refs and autoload_user_set, > > > > > > for BPF_PROG_TYPE_STRUCT_OPS, how about deciding to load it or not by checking > > > prog->attach_btf_id (non zero) alone. The prog->attach_btf_id is now decided at > > > load time and is only set if it is used by at least one autocreate map, if I > > > read patch 2 & 3 correctly. > > > > > > Meaning ignore prog->autoload*. Load the struct_ops prog as long as it is used > > > by one struct_ops map with autocreate == true. > > > > > > If the struct_ops prog is not used in any struct_ops map, the bpf prog cannot be > > > loaded even the autoload is set. If bpf prog is used in a struct_ops map and its > > > autoload is set to false, the struct_ops map will be in broken state. Thus, > > > > We can easily detect this condition and report meaningful error. > > > > > prog->autoload does not fit very well with struct_ops prog and may as well > > > depend on whether the struct_ops prog is used by a struct_ops map alone? > > > > I think it's probably fine from a usability standpoint to disable > > loading the BPF program if its struct_ops map was explicitly set to > > not auto-create. It's a bit of deviation from other program types, but > > in practice this logic will make it easier for users. > > > > One question I have, though, is whether we should report as an error a > > stand-alone struct_ops BPF program that is not used from any > > struct_ops map? Or should we load it nevertheless? Or should we > > silently not load it? > > > > I feel like silently not loading is the worst behavior here. So either > > loading it anyway or reporting an error would be my preference, > > probably. > > The following properties of the struct_ops program are set based on > the corresponding struct_ops map: > - attach_btf_id - BTF id of the kernel struct_ops type; > - expected_attach_type - member index of function pointer inside > the kernel type. > > No corresponding map means above fields are not set, > means program fails to load with error report. > > So I think it is fine to try loading such programs w/o any additional > processing. But Martin is proposing to *not load* programs if their attach_btf_id is not set. Which is why I'm asking if we should distinguish situations where a BPF program is stand-alone (never was referenced from struct_ops map) vs auto-disabling it because struct_ops map that it was referenced from was explicitly disabled.
