On Tue, Dec 8, 2020 at 8:58 AM Nguyen, Anthony L
[off-list ref] wrote:

On Mon, 2020-11-23 at 17:11 -0800, Alexander Duyck wrote:

quoted

On Mon, Nov 23, 2020 at 3:21 PM Jesse Brandeburg
[off-list ref] wrote:

quoted

Alexander Duyck wrote:

quoted

quoted

quoted

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.

quoted

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.

quoted

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.

- Alex