Date:   Wed, 9 Dec 2020 18:23:13 +0000
From:   "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
To:     "alexander.duyck@...il.com" <alexander.duyck@...il.com>
CC:     "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Cao, Chinh T" <chinh.t.cao@...el.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Valiquette, Real" <real.valiquette@...el.com>,
        "Behera, BrijeshX" <brijeshx.behera@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [net-next v3 05/15] ice: create flow profile

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.nguyen@...el.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.nguyen@...el.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.brandeburg@...el.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

Powered by blists - more mailing lists