Re: [RFC v2 net-next 01/12] net: introduce BPF_XDP_EGRESS attach type for XDP

[Prashant Bhole <bholeprashant.oss@gmail.com>]

On 1/7/2020 8:35 PM, Toke Høiland-Jørgensen wrote:
> Prashant Bhole <prashantbhole.linux@gmail.com> writes:
> 
>> On 12/27/2019 11:27 PM, Jesper Dangaard Brouer wrote:
>>> On Thu, 26 Dec 2019 11:31:49 +0900
>>> Prashant Bhole <prashantbhole.linux@gmail.com> wrote:
>>>
>>>> This patch introduces a new bpf attach type BPF_XDP_EGRESS. Programs
>>>> having this attach type will be allowed to run in the tx path. It is
>>>> because we need to prevent the programs from accessing rxq info when
>>>> they are running in tx path. Verifier can reject the programs those
>>>> have this attach type and trying to access rxq info.
>>>>
>>>> Patch also introduces a new netlink attribute IFLA_XDP_TX which can
>>>> be used for setting XDP program in tx path and to get information of
>>>> such programs.
>>>>
>>>> Drivers those want to support tx path XDP needs to handle
>>>> XDP_SETUP_PROG_TX and XDP_QUERY_PROG_TX cases in their ndo_bpf.
>>>
>>> Why do you keep the "TX" names, when you introduce the "EGRESS"
>>> attachment type?
>>>
>>> Netlink attribute IFLA_XDP_TX is particularly confusing.
>>>
>>> I personally like that this is called "*_XDP_EGRESS" to avoid confusing
>>> with XDP_TX action.
>>
>> It's been named like that because it is likely that a new program
>> type tx path will be introduced later. It can re-use IFLA_XDP_TX
>> XDP_SETUP_PROG_TX, XDP_QUERY_PROG_TX. Do think that it should not
>> be shared by two different type of programs?
> 
> I agree that the *PROG_TX stuff is confusing.

Ok. It seems s/TX/EGRESS is good for now.

> 
> Why not just keep the same XDP attach command, and just make this a new
> attach mode? I.e., today you can do
> 
> bpf_set_link_xdp_fd(ifindex, prog_fd, XDP_FLAGS_DRV_MODE);
> 
> so for this, just add support for:
> 
> bpf_set_link_xdp_fd(ifindex, prog_fd, XDP_FLAGS_EGRESS_MODE);
> 
> No need for a new command/netlink attribute. We already support multiple
> attach modes (HW+DRV), so this should be a straight-forward extension,
> no?

Initially we had implemented it the same way. I am ok with this way too.
- new attachment flag BPF_XDP_EGRESS for verifier purpose
- new xdp flag XDP_FLAGS_EGRESS for libbpf


Thanks