Re: [net-next v3 05/15] ice: create flow profile

["Nguyen, Anthony L" <anthony.l.nguyen@intel.com>]

On Fri, 2020-11-13 at 15:56 -0800, Alexander Duyck wrote:
> On Fri, Nov 13, 2020 at 1:46 PM Tony Nguyen <
> anthony.l.nguyen@intel.com> wrote:
> > 
> > From: Real Valiquette <real.valiquette@intel.com>
> > 
> > Implement the initial steps for creating an ACL filter to support
> > ntuple
> > masks. Create a flow profile based on a given mask rule and program
> > it to
> > the hardware. Though the profile is written to hardware, no actions
> > are
> > associated with the profile yet.
> > 
> > Co-developed-by: Chinh Cao <chinh.t.cao@intel.com>
> > Signed-off-by: Chinh Cao <chinh.t.cao@intel.com>
> > Signed-off-by: Real Valiquette <real.valiquette@intel.com>
> > Co-developed-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Tested-by: Brijesh Behera <brijeshx.behera@intel.com>
> 
> So I see two big issues with the patch.
> 
> First it looks like there is an anti-pattern of defensive NULL
> pointer
> checks throughout. Those can probably all go since all of the callers
> either use the pointer, or verify it is non-NULL before calling the
> function in question.

I'm removing those checks that you pointed out and some others as well.

> 
> In addition the mask handling doens't look right to me. It is calling
> out a partial mask as being the only time you need an ACL and I would
> think it is any time you don't have a full mask for all
> ports/addresses since a flow director rule normally pulls in the full
> 4 tuple based on ice_ntuple_set_input_set() .

Commented below as well.

<snip>

> > +/**
> > + * ice_is_acl_filter - Checks if it's a FD or ACL filter
> > + * @fsp: pointer to ethtool Rx flow specification
> > + *
> > + * If any field of the provided filter is using a partial mask
> > then this is
> > + * an ACL filter.
> > + *
> 
> 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.


> > + * Returns true if ACL filter otherwise false.
> > + */
> > +static bool ice_is_acl_filter(struct ethtool_rx_flow_spec *fsp)
> > +{
> > +       struct ethtool_tcpip4_spec *tcp_ip4_spec;
> > +       struct ethtool_usrip4_spec *usr_ip4_spec;
> > +
> > +       switch (fsp->flow_type & ~FLOW_EXT) {
> > +       case TCP_V4_FLOW:
> > +       case UDP_V4_FLOW:
> > +       case SCTP_V4_FLOW:
> > +               tcp_ip4_spec = &fsp->m_u.tcp_ip4_spec;
> > +
> > +               /* IP source address */
> > +               if (tcp_ip4_spec->ip4src &&
> > +                   tcp_ip4_spec->ip4src != htonl(0xFFFFFFFF))
> > +                       return true;
> > +
> > +               /* IP destination address */
> > +               if (tcp_ip4_spec->ip4dst &&
> > +                   tcp_ip4_spec->ip4dst != htonl(0xFFFFFFFF))
> > +                       return true;
> > +
> 
> Instead of testing this up here you could just skip the break and
> fall
> through since the source and destination IP addresses occupy the same
> spots on usr_ip4_spec and tcp_ip4_spec. You could probably also just
> use tcp_ip4_spec for the entire test.

Will make this change.

Thanks,
Tony