On Wed, Nov 20, 2024, at 8:07 AM, Alexei Starovoitov wrote:
> On Wed, Nov 20, 2024 at 7:55 AM Daniel Xu <dxu@xxxxxxxxx> wrote:
>>
>>
>>
>> On Sat, Nov 16, 2024, at 2:17 PM, Alexei Starovoitov wrote:
>> > On Tue, Oct 29, 2024 at 11:36 PM Daniel Xu <dxu@xxxxxxxxx> wrote:
>> >>
>> >> Hey Daniel,
>> >>
>> >> On Wed, Aug 23, 2023, at 9:08 AM, Daniel Xu wrote:
>> >> > This patchset changes the behavior of TC and XDP hooks during attachment
>> >> > such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
>> >> > uref taken.
>> >> >
>> >> > The goal behind this change is to try and prevent confusion for the
>> >> > majority of use cases. The current behavior where when the last uref is
>> >> > dropped the prog array map is emptied is quite confusing. Confusing
>> >> > enough for there to be multiple references to it in ebpf-go [0][1].
>> >> >
>> >> > Completely solving the problem is difficult. As stated in c9da161c6517
>> >> > ("bpf: fix clearing on persistent program array maps"), it is
>> >> > difficult-to-impossible to walk the full dependency graph b/c it is too
>> >> > dynamic.
>> >> >
>> >> > However in practice, I've found that all progs in a tailcall chain
>> >> > share the same prog array map. Knowing that, if we take a uref on any
>> >> > used prog array map when the program is attached, we can simplify the
>> >> > majority use case and make it more ergonomic.
>> >
>> > Are you proposing to inc map uref when prog is attached?
>> >
>> > But that re-adds the circular dependency that uref concept is solving.
>> > When prog is inserted into prog array prog refcnt is incremented.
>> > So if prog also incremented uref. The user space can exit
>> > but prog array and progs will stay there though nothing is using them.
>> > I guess I'm missing the idea.
>>
>> IIRC the old-style tc/xdp attachment is the one incrementing the uref.
>
> uref is incremented when FD is given to user space and
> file->release() callback decrements uref.
>
> I don't think any of the attach operations mess with uref.
> At least they shouldn't.
None yet. My patch was adding it. It's fine if it's too much of a hack -
was just an idea.
