On Tue, 2020-12-08 at 14:22 -0800, Alexander Duyck wrote: > On Tue, Dec 8, 2020 at 2:01 PM Nguyen, Anthony L > <anthony.l.ngu...@intel.com> wrote: > > > > On Tue, 2020-12-08 at 11:00 -0800, Alexander Duyck wrote: > > > On Tue, Dec 8, 2020 at 8:58 AM Nguyen, Anthony L > > > <anthony.l.ngu...@intel.com> wrote: > > > > > > > > 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. > > > > > > No, I get that. The question I have is what happens if you try to > > > input a second input set. With ixgbe we triggered an error for > > > trying > > > to change input sets. I'm wondering if you trigger an error on > > > adding > > > a different input set or if you just invalidate the existing > > > rules. > > > > > > > > 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. > > > > > > So a better question here is what happens if you do a rule with > > > src-port 8500, and a second rule with dst-port 8500? Does the > > > second > > > rule fail or does it invalidate the first. If it invalidates the > > > first > > > then that would be a bug. > > > > The second rule fails and a message is output to dmesg. > > > > ethtool -N ens801f0 flow-type tcp4 src-ip 192.168.0.10 dst-ip > > 192.168.0.20 dst-port 8500 action 10 > > rmgr: Cannot insert RX class rule: Operation not supported > > Ugh. I really don't like the choice to use EOPNOTSUPP as the return > value for a mask case. It really should have been something like an > EBUSY or EINVAL since you are trying to overwrite an already written > mask so you can change the field configuration. > > > dmesg: > > ice 0000:81:00.0: Failed to add filter. Flow director filters on > > each > > port must have the same input set. > > Okay, so this is the behavior you see with Flow Director. If you > don't > apply a partial mask it fails to add the second rule. > > > > > > 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 > > > > > > The test here is to set-up two rules and verify each of them and > > > one > > > case that fails both. Same thing for the test above. Basically we > > > should be able to program multiple ACL rules with different masks > > > and > > > that shouldn't be an issue up to some limit I would imagine. Same > > > thing for flow director rules. After the first you should not be > > > able > > > to provide a flow director rule with a different input mask. > > > > I did this: > > > > 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 > > ethtool -N ens801f0 flow-type tcp4 src-ip 192.168.0.10 dst-ip > > 192.168.0.20 src-port 8000 m 0x2 action 20 > > > > Sending traffic to port 9000 and 9001 goes to queue 15 > > Sending traffic to port 8000 and 8002 goes to queue 20 > > Sending traffic to port 8001 and 8500 goes to neither of the queues > > Doing the same thing with a mask works. I could add src-port with a > mask in one rule, and I could add dst-port with a mask in another. > Can > you see the inconsistency here?
Thanks for the reviews Alex. I see your point. I'm going to drop the ACL patches from this series and send the other patches while we look into this. -Tony
