Comments regarding Flow Director support in PMD IXGBE

Comments regarding Flow Director support in PMD IXGBE

[Robert Sanford]

Hi,

We would like your opinions on the following observations, regarding
the 82599 Flow Director support.


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).


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.

--
Regards,
Robert

Re: Comments regarding Flow Director support in PMD IXGBE

[Maxime Leroy]

Hi Robert,

On Fri, Jan 3, 2014 at 8:52 PM, Robert Sanford <rsanford-mDvk1qcdE6VWk0Htik3J/w@public.gmane.org> 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

Re: Comments regarding Flow Director support in PMD IXGBE

[Robert Sanford]

Hello Maxime,

Thank you for taking the time to research these flow director issues and a
more complete solution.

On Fri, Jan 10, 2014 at 8:36 AM, Maxime Leroy <maxime.leroy-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
 wrote:
> 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)
...
> 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.
...
> The testpmd application need to be updated in consequence to provide ip
mask
> in network order (in lib/librte_cmdline/cmdline.c):
...
> Would you like to provide and test a patch to fix this issue, please ?


Yes, we will do as you suggest:
1. Test with IPv6 traffic.
2. Make appropriate changes to flow director code and testpmd.
3. Update comments in the structure definitions to indicate byte order.
4. Provide the patch.


--
Regards,
Robert