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.ngu...@intel.com> wrote:
> >
> > From: Real Valiquette <real.valique...@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....@intel.com>
> > Signed-off-by: Chinh Cao <chinh.t....@intel.com>
> > Signed-off-by: Real Valiquette <real.valique...@intel.com>
> > Co-developed-by: Tony Nguyen <anthony.l.ngu...@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.ngu...@intel.com>
> > Tested-by: Brijesh Behera <brijeshx.beh...@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
