On 6/9/21 4:08 PM, Daniel Borkmann wrote:
> Hi Rumen, hi David,
>
> (please avoid top-posting)
>
> On 6/9/21 11:56 PM, Rumen Telbizov wrote:
>> List,
>>
>> For what it's worth I patched the structure locally by introducing a
>> new __u32 mark field
>> to the structure and adding the proper assignment of the field in
>> filter.c. Recompiled without any issues.
>> With that patch a bpf lookup matches ip rule that contains fwmark.
>>
>> Still interested to know how much of a performance penalty adding an 4
>> bytes to the
>> structure brings. I'd certainly vote for adding at least the firewall
>> mark to the set of fields used in the lookup.
>
> I agree with David here that performance of the helper is paramount.
> As a side-note, we should probably add a build_bug_on() to ensure that
> the size of struct bpf_fib_lookup will stay at 64b / one cacheline.
that's the key point on performance - crossing a cacheline. I do not
have performance data at hand, but it is a substantial hit. That is why
the struct is so overloaded (and complicated for a uapi) with the input
vs output setting.
Presumably you are parsing the packet to id a flow to find the mark that
should be used with the FIB lookup. correct? Hardware hints will make
that part easier whenever that feature actually lands and if the NIC can
add the mark.
>
> That said, given h_vlan_proto/h_vlan_TCI are both output parameters,
> maybe we could just union the two fields with a __u32 mark extension
> that we then transfer into the flowi{4,6}?
That is one option.
I would go for a union on sport and/or dport. It is a fair tradeoff to
request users to pick one - policy routing based on L4 ports or fwmark.
A bit harder to do with a straight up union at this point, but we could
also limit the supported fwmark to 16-bits. Hard choices have to be made.
