Re: [dpdk-dev] Comments regarding Flow Director support in PMD IXGBE

[Maxime Leroy <maxime.leroy@6wind.com>]

Hi Robert,

On Fri, Jan 3, 2014 at 8:52 PM, Robert Sanford <rsanford@prolexic.com> wrote:
> Issue #1:
> Our reading of the 82599 data sheet leads us to believe that
> Flow Director can simultaneously handle *both* IPv4 and IPv6 filters,
> with separate filter rules, of course.
>
> Thus, at the bottom of ixgbe_fdir.c:fdir_set_input_mask_82599( ),
> we could remove the "if (!input_mask->set_ipv6_mask)" / "else"
> around the setting of FDIRSIP4M, FDIRDIP4M, and FDIRIP6M.
> (This would also eliminate the need for the set_ipv6_masks flag itself.)
>
> We performed limited testing on this change. We have successfully
> added both IPv4 and IPv6 signature filters, but so far have only
> exercised them with IPv4 traffic.
>
> One would think that the designers of this chip feature envisioned
> users filtering mixed traffic (both IPv4 and IPv6).

By reading the 82599 datasheet, I have the same analyze than you,
the flow director masks seems to be independent for ipv4 and ipv6.

But it will be nice to have a small test with ipv6 traffic to be sure
about this point.

Would you like to provide a patch to remove this useless "if" please ?

(Note: the set_ipv6_mask field of the input_mask structure need to be
removed too)

> Issue #2:
> Apparently, API rte_eth_dev_fdir_set_masks( ) expects IPv4 address
> and port masks in host-byte-order (little-endian), while
> rte_eth_dev_fdir_add_signature_filter( ) expects IPv4 addresses and
> ports in network-byte-order (big-endian).
>
> (Contrast the writing into IXGBE_FDIRSIP4M in ixgbe_fdir.c:
> fdir_set_input_mask_82599( ), versus ixgbe/ixgbe_82599.c:
> ixgbe_fdir_set_input_mask_82599( ). The former includes an extra
> IXGBE_NTOHL( ) on the mask's complement.)
>
> Not knowing this made it a bit tricky to get signature filters working
> properly. Perhaps it is too late to change the byte-ordering in the
> (set masks) API? Whether we change it or not, we probably should
> at least document these details, to avoid confusion.

First, you probably know this point, a good way to test flow director in dpdk is
to use the testpmd application.

And it's also a good example to understand how to use rte_eth_dev_fdir_* api.

So by reading the app/test-pmd/cmdline.c file, I can understand
that the mask is parsed in little-endian for rte_eth_dev_fdir_set_masks.
And the src/dst ip addresses are parsed in big-endian for
rte_eth_dev_fdir_add_signature_filter.

Thus I agree with your analyze, the fdir api is not coherent.
I think all the parameters of the fdir api should be in network order.

+ About a patch to fix the api:

As you said, IXGBE_NTOHL need to be removed and IXGBE_WRITE_REG need
to be used instead of IXGBE_WRITE_REG_BE32 (in
lib/librte_pmd_ixgbe/ixgbe_fdir.c):

          /* Store source and destination IPv4 masks (big-endian) */
 -          IXGBE_WRITE_REG_BE32(hw, IXGBE_FDIRSIP4M,
 -                IXGBE_NTOHL(~input_mask->src_ipv4_mask));
 +              IXGBE_WRITE_REG(hw, IXGBE_FDIRSIP4M,
 +                ~input_mask->src_ipv4_mask);

The testpmd application need to be updated in consequence to provide ip mask
in network order (in lib/librte_cmdline/cmdline.c):

  - fdir_masks.dst_ipv4_mask = res->ip_dst_mask;
  - fdir_masks.src_ipv4_mask = res->ip_src_mask;
  + fdir_masks.dst_ipv4_mask = rte_cpu_to_be_32(res->ip_dst_mask);
  + fdir_masks.dst_ipv4_mask = rte_cpu_to_be_32(res->ip_dst_mask);

Would you like to provide and test a patch to fix this issue, please ?

Thanks. Best Regards,

---------------------------
Maxime Leroy
maxime.leroy@6wind.com