Re: Comments regarding Flow Director support in PMD IXGBE

Hi Robert,

On Fri, Jan 3, 2014 at 8:52 PM, Robert Sanford [off-list ref] 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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org