Re: [RFC] Generic flow director/filtering/classification API

[John Fastabend <john.fastabend@gmail.com>]

[...]

>>>>>> The proposal looks very good.  It satisfies most of the features
>>>>>> supported by Chelsio NICs.  We are looking for suggestions on exposing
>>>>>> more additional features supported by Chelsio NICs via this API.
>>>>>>
>>>>>> Chelsio NICs have two regions in which filters can be placed -
>>>>>> Maskfull and Maskless regions.  As their names imply, maskfull region
>>>>>> can accept masks to match a range of values; whereas, maskless region
>>>>>> don't accept any masks and hence perform a more strict exact-matches.
>>>>>> Filters without masks can also be placed in maskfull region.  By
>>>>>> default, maskless region have higher priority over the maskfull region.
>>>>>> However, the priority between the two regions is configurable.
>>>>>
>>>>> I understand this configuration affects the entire device. Just to be clear,
>>>>> assuming some filters are already configured, are they affected by a change
>>>>> of region priority later?
>>>>>
>>>>
>>>> Both the regions exist at the same time in the device.  Each filter can
>>>> either belong to maskfull or the maskless region.
>>>>
>>>> The priority is configured at time of filter creation for every
>>>> individual filter and cannot be changed while the filter is still
>>>> active. If priority needs to be changed for a particular filter then,
>>>> it needs to be deleted first and re-created.
>>>
>>> Could you model this as two tables and add a table_id to the API? This
>>> way user space could populate the table it chooses. We would have to add
>>> some capabilities attributes to "learn" if tables support masks or not
>>> though.
>>>
>>
>> This approach sounds interesting.
> 
> Now I understand the idea behind these tables, however from an application
> point of view I still think it's better if the PMD could take care of flow
> rules optimizations automatically. Think about it, PMDs have exactly a
> single kind of device they know perfectly well to manage, while applications
> want the best possible performance out of any device in the most generic
> fashion.

The problem is keeping priorities in order and/or possibly breaking
rules apart (e.g. you have an L2 table and an L3 table) becomes very
complex to manage at driver level. I think its easier for the
application which has some context to do this. The application "knows"
if its a router for example will likely be able to pack rules better
than a PMD will.

> 
>>> I don't see how the PMD can sort this out in any meaningful way and it
>>> has to be exposed to the application that has the intelligence to 'know'
>>> priorities between masks and non-masks filters. I'm sure you could come
>>> up with something but it would be less than ideal in many cases I would
>>> guess and we can't have the driver getting priorities wrong or we may
>>> not get the correct behavior.
> 
> It may be solved by having the PMD maintain a SW state to quickly know which
> rules are currently created and in what state the device is so basically the
> application doesn't have to perform this work.
> 
> This API allows applications to express basic needs such as "redirect
> packets matching this pattern to that queue". It must not deal with HW
> details and limitations in my opinion. If a request cannot be satisfied,
> then the rule cannot be created. No help from the application must be
> expected by PMDs, otherwise it opens the door to the same issues as the
> legacy filtering APIs.

This depends on the application and what/how it wants to manage the
device. If the application manages a pipeline with some set of tables,
then mapping this down to a single table, which then the PMD has to
unwind back to a multi-table topology to me seems like a waste.

> 
> [...]
>>>> Unfortunately, our maskfull region is extremely small too compared to
>>>> maskless region.
>>>>
>>>
>>> To me this means a userspace application would want to pack it
>>> carefully to get the full benefit. So you need some mechanism to specify
>>> the "region" hence the above table proposal.
>>>
>>
>> Right. Makes sense.
> 
> I do not agree, applications should not be aware of it. Note this case can
> be handled differently, so that rules do not have to be moved back and forth
> between both tables. If the first created rule requires a maskfull entry,
> then all subsequent rules will be entered into that table. Otherwise no
> maskfull entry can be created as long as there is one maskless entry. When
> either table is full, no more rules may be added. Would that work for you?
> 

Its not about mask vs no mask. The devices with multiple tables that I
have don't have this mask limitations. Its about how to optimally pack
the rules and who implements that logic. I think its best done in the
application where I have the context.

Is there a way to omit the table field if the PMD is expected to do
a best effort and add the table field if the user wants explicit
control over table mgmt. This would support both models. I at least
would like to have explicit control over rule population in my pipeline
for use cases where I'm building a pipeline on top of the hardware.

>> [...]
>>>>> Now about this "promisc" match criteria, it can be added as a new meta
>>>>> pattern item (4.1.3 Meta item types). Do you want it to be defined from the
>>>>> start or add it later with the related code in your PMD?
>>>>>
>>>>
>>>> It could be added as a meta item.  If there are other interested
>>>> parties, it can be added now.  Otherwise, we'll add it with our filtering
>>>> related code.
>>>>
>>>
>>> hmm I guess by "promisc" here you mean match packets received from the
>>> wire before they have been switched by the silicon?
>>>
>>
>> Match packets received from wire before they have been switched by
>> silicon, and which also includes packets not destined for DUT and were
>> still received due to interface being in promisc mode.
> 
> I think it's fine, but we'll have to precisely define what happens when a
> packet matched with such pattern is part of a terminating rule. For instance
> if it is duplicated by HW, then the rule cannot be terminating.
> 
> [...]
>>>> This raises another interesting question.  What should the PMD do
>>>> if it has support to only a subset of fields in the particular item?
>>>>
>>>> For example, if a rule has been sent to match IP fragmentation along
>>>> with several other IPv4 fields, and if the underlying hardware doesn't
>>>> support matching based on IP fragmentation, does the PMD reject the
>>>> complete rule although it could have done the matching for rest of the
>>>> IPv4 fields?
>>>
>>> I think it has to fail the command other wise user space will not have
>>> any way to understand that the full match criteria can not be met and
>>> we will get different behavior for the same applications on different
>>> nics depending on hardware feature set. This will most likely break
>>> applications so we need the error IMO.
>>>
>>
>> Ok. Makes sense.
> 
> Yes, I fully agree with this.
> 
>>>>>> - Match range of physical ports on the NIC in a single rule via masks.
>>>>>>   For ex: match all UDP packets coming on ports 3 and 4 out of 4
>>>>>>   ports available on the NIC.
>>>>>
>>>>> Applications create flow rules per port, I'd thus suggest that the PMD
>>>>> should detect identical rules created on different ports and aggregate them
>>>>> as a single HW rule automatically.
>>>>>
>>>>> If you think this approach is not right, the alternative is a meta pattern
>>>>> item that provides a list of ports. I'm not sure this is the right approach
>>>>> considering it would most likely not be supported by most NICs. Applications
>>>>> may not request it explicitly.
>>>>>
>>>>
>>>> Aggregating via PMD will be expensive operation since it would involve:
>>>> - Search of existing filters.
>>>> - Deleting those filters.
>>>> - Creating a single combined filter.
>>>>
>>>> And all of above 3 operations would need to be atomic so as not to
>>>> affect existing traffic which is hitting above filters.
> 
> Atomicity may not be a problem if the PMD makes sure the new combined rule
> is inserted before the others, so they do not need to be removed either.
> 
>>>> Adding a
>>>> meta item would be a simpler solution here.
> 
> Yes, clearly.
> 
>>> For this adding a meta-data item seems simplest to me. And if you want
>>> to make the default to be only a single port that would maybe make it
>>> easier for existing apps to port from flow director. Then if an
>>> application cares it can create a list of ports if needed.
>>>
>>
>> Agreed.
> 
> However although I'm not opposed to adding dedicated meta items, remember
> applications will not automatically benefit from the increased performance
> if a single PMD implements this feature, their maintainers will probably not
> bother with it.
> 

Unless as we noted in other thread the application is closely bound to
its hardware for capability reasons. In this case it would make sense
to implement.

>>>>>> - Match range of Physical Functions (PFs) on the NIC in a single rule
>>>>>>   via masks. For ex: match all traffic coming on several PFs.
>>>>>
>>>>> The PF and VF pattern items assume there is a single PF associated with a
>>>>> DPDK port. VFs are identified with an ID. I basically took the same
>>>>> definitions as the existing filter types, perhaps this is not enough for
>>>>> Chelsio adapters.
>>>>>
>>>>> Do you expose more than one PF for a DPDK port?
>>>>>
>>>>> Anyway, I'd suggest the same approach as above, automatic aggregation of
>>>>> rules for performance reasons, otherwise new or updated PF/VF pattern items,
>>>>> in which case it would be great if you could provide ideal structure
>>>>> definitions for this use case.
>>>>>
>>>>
>>>> In Chelsio hardware, all the ports of a device are exposed via single
>>>> PF4. There could be many VFs attached to a PF.  Physical NIC functions
>>>> are operational on PF4, while VFs can be attached to PFs 0-3.
>>>> So, Chelsio hardware doesn't remain tied on a PF-to-Port, one-to-one
>>>> mapping assumption.
>>>>
>>>> There already seems to be a PF meta-item, but it doesn't seem to accept
>>>> any "spec" and "mask" field.  Similarly, the VF meta-item doesn't
>>>> seem to accept a "mask" field.  We could probably enable these fields
>>>> in the PF and VF meta-items to allow configuration.
>>>
>>> Maybe a range field would help here as well? So you could specify a VF
>>> range. It might be one of the things to consider adding later though if
>>> there is no clear use for it now.
>>>
>>
>> VF-value and VF-mask would help to achieve the desired filter.
>> VF-mask would also enable to specify a range of VF values.
> 
> Like John, I think a range or even a list instead of a mask would be better,
> the PMD can easily create a mask from that if necessary. Reason is that
> we've always had bad experiences with bit-fields, they're always too short
> at some point and we would like to avoid having to break the ABI to update
> existing pattern items later.

Agreed avoiding bit-fields is a good idea.

> 
> Also while I don't think this is the case yet, perhaps it will be a good
> idea for PFs/VFs to have global unique IDs, just like DPDK ports.
> 
> Thanks.
>