On Wed, Jul 1, 2020 at 7:17 PM Roman Gushchin <guro@xxxxxx> wrote:
> I'm slightly concerned about changing the existing behavior without
> providing the backward compatibility. Idk how many users will actually
> suffer, but it might be an issue. Of course, it's up to maintainers
> to decide whether it's ok or not to break the existing behavior
> in this case.
This was considered. The problem with having full backwards
compatibility is that, one, the kernel code can be quite complicated.
If it's a 'legacy CGROUP_STORAGE' do this, else do that. Due to the
difference between the lifetime it would be sprinkled into a few
functions. Second, you would either need a flag to the map or a new
map type to indicate the legacy behavior. The former would be drawing
another bit from the limited bit-flags, and the latter, there would be
two more types, per-CPU and non-per-CPU. Perhaps per-CPU
shared-between-program is not useful and we would only implement
non-per-CPU shared-between-program to not repeat a lot of complexity.
> As I understand from the code, it's not called when the cgroup freed,
> but when cgroup_bpf is freed (i.e. from cgroup_bpf_release().
> It's actually very good, just has to be fixed in the description above.
Didn't realize it was different. Thanks. Will fix in v2.
> > pl = kmalloc(sizeof(*pl), GFP_KERNEL);
> > - if (!pl) {
> > - bpf_cgroup_storages_free(storage);
> > - return -ENOMEM;
> > - }
>
> Hm, why -ENOMEM handling has been removed here?
oof, my oversight when undoing some refactoring. Will fix in v2.
YiFei Zhu
