On Mon, 2020-11-23 at 17:11 -0800, Alexander Duyck wrote: > On Mon, Nov 23, 2020 at 3:21 PM Jesse Brandeburg > <jesse.brandeb...@intel.com> wrote: > > > > Alexander Duyck wrote: > > > > > > > I'm not sure this logic is correct. Can the flow director > > > > > rules > > > > > handle > > > > > a field that is removed? Last I knew it couldn't. If that is > > > > > the case > > > > > you should be using ACL for any case in which a full mask is > > > > > not > > > > > provided. So in your tests below you could probably drop the > > > > > check > > > > > for > > > > > zero as I don't think that is a valid case in which flow > > > > > director > > > > > would work. > > > > > > > > > > > > > I'm not sure what you meant by a field that is removed, but > > > > Flow > > > > Director can handle reduced input sets. Flow Director is able > > > > to handle > > > > 0 mask, full mask, and less than 4 tuples. ACL is needed/used > > > > only when > > > > a partial mask rule is requested. > > > > > > So historically speaking with flow director you are only allowed > > > one > > > mask because it determines the inputs used to generate the hash > > > that > > > identifies the flow. So you are only allowed one mask for all > > > flows > > > because changing those inputs would break the hash mapping. > > > > > > Normally this ends up meaning that you have to do like what we > > > did in > > > ixgbe and disable ATR and only allow one mask for all inputs. I > > > believe for i40e they required that you always use a full 4 > > > tuple. I > > > didn't see something like that here. As such you may want to > > > double > > > check that you can have a mix of flow director rules that are > > > using 1 > > > tuple, 2 tuples, 3 tuples, and 4 tuples as last I knew you > > > couldn't. > > > Basically if you had fields included they had to be included for > > > all > > > the rules on the port or device depending on how the tables are > > > set > > > up. > > > > The ice driver hardware is quite a bit more capable than the ixgbe > > or > > i40e hardware, and uses a limited set of ACL rules to support > > different > > sets of masks. We have some limits on the number of masks and the > > number of fields that we can simultaneously support, but I think > > that is pretty normal for limited hardware resources. > > > > Let's just say that if the code doesn't work on an E810 card then > > we > > messed up and we'll have to fix it. :-) > > > > Thanks for the review! Hope this helps... > > I gather all that. The issue was the code in ice_is_acl_filter(). > Basically if we start dropping fields it will not trigger the rule to > be considered an ACL rule if the field is completely dropped. > > So for example I could define 4 rules, one that ignores the IPv4 > source, one that ignores the IPv4 destination, one that ignores the > TCP source port, and one that ignores the TCP destination port.
We have the limitation that you can use one input set at a time so any of these rules could be created but they couldn't exist concurrently. > With > the current code all 4 of those rules would be considered to be > non-ACL rules because the mask is 0 and not partial. Correct. I did this to test Flow Director: 'ethtool -N ens801f0 flow-type tcp4 src-ip 192.168.0.10 dst-ip 192.168.0.20 src-port 8500 action 10' and sent traffic matching this. Traffic correctly went to queue 10. > If I do the same > thing and ignore all but one bit then they are all ACL rules. Also correct. I did as follows: 'ethtool -N ens801f0 flow-type tcp4 src-ip 192.168.0.10 dst-ip 192.168.0.20 src-port 9000 m 0x1 action 15' Sending traffic to port 9000 and 90001, traffic went to queue 15 Sending traffic to port 8000 and 90002, traffic went to other queues Thanks, Tony > In > addition I don't see anything telling flow director it can ignore > certain inputs over verifying the mask so I am assuming that the > previously mentioned rules that drop entire fields would likely not > work with Flow Director. > > Anyway I just wanted to point that out as that would be an issue > going > forward and it seems like it would be easy to fix by simply just > rejecting rules where the required flow director fields are not > entirely masked in. > > - Alex
