Hi Wei,

Hi, Konstantin
Thanks for your feedback.

quoted

-----Original Message-----
From: Ananyev, Konstantin
Sent: Thursday, March 15, 2018 5:48 AM
To: Dai, Wei <redacted>; Lu, Wenzhuo <redacted>
Cc: dev@dpdk.org
Subject: RE: [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API

Hi Wei,

quoted

-----Original Message-----
From: Dai, Wei
Sent: Wednesday, March 7, 2018 1:06 PM
To: Lu, Wenzhuo <redacted>; Ananyev, Konstantin
[off-list ref]
Cc: dev@dpdk.org; Dai, Wei <redacted>
Subject: [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API

Ethdev Rx offloads API has changed since:
commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") This
commit support the new Rx offloads API.

Signed-off-by: Wei Dai <redacted>
---
 drivers/net/ixgbe/ixgbe_ethdev.c          |  93 +++++++++--------
 drivers/net/ixgbe/ixgbe_ipsec.c           |   8 +-
 drivers/net/ixgbe/ixgbe_rxtx.c            | 163

++++++++++++++++++++++++++----

quoted

 drivers/net/ixgbe/ixgbe_rxtx.h            |   3 +
 drivers/net/ixgbe/ixgbe_rxtx_vec_common.h |   2 +-
 drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c   |   2 +-
 6 files changed, 205 insertions(+), 66 deletions(-)

+uint64_t
+ixgbe_get_rx_queue_offloads(struct rte_eth_dev *dev) {
+	uint64_t offloads;
+	struct ixgbe_hw *hw =
+IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+	offloads = DEV_RX_OFFLOAD_HEADER_SPLIT;

As I can see I ixgbe all header_split code is enabled only if
RTE_HEADER_SPLIT_ENABLE is on.
It is off by default and I doubt anyone really using it these days.
So I think the best thing would be not to advertise
DEV_RX_OFFLOAD_HEADER_SPLIT for ixgbe at all, and probably remove
related code.
If you'd prefer to keep it, then at least we should set that capability only at
#ifdef RTE_HEADER_SPLIT_ENABLE.
Another thing - 	it should be per port, not per queue.
Thought I think better is just to remove it completely.

I will set this header splitting capability in #ifdef RTE_HEADER_SPLIT_ENABLE in my next patch set.
I think it is a per queue capability as it can be configured on the register IXGBE_SRRCTL of every Rx queue
In this code line: IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(rxq->reg_idx), srrctl); in ixgbe_dev_rx_init( ).
Same case is also in the code line: IXGBE_WRITE_REG(hw, IXGBE_VFSRRCTL(i), srrctl); in ixgbevf_dev_rx_init( ).

Yes, HW can enable/disable it on a per queue basis.
Though it affects rx function selection, and as right now we have one rx function per device -
That's why it looks to me more like a per port offload.
Though I believe these days ixgbe PMD doesn't support it properly anyway 
(we always set rxd.hdr_addr to zero) - so probably better to remove it at all.

quoted

quoted

+static int
+ixgbe_check_rx_queue_offloads(struct rte_eth_dev *dev, uint64_t
+requested) {
+	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
+	uint64_t queue_supported = ixgbe_get_rx_queue_offloads(dev);
+	uint64_t port_supported = ixgbe_get_rx_port_offloads(dev);
+
+	if ((requested & (queue_supported | port_supported)) != requested)
+		return 0;
+
+	if ((port_offloads ^ requested) & port_supported)

Could you explain a bit more what are you cheking here?
As I can see:
 (port_offloads ^ requested) - that's a diff between already set and newly
requested offloads.
Then you check if that diff consists of supported by port offloads, and if yes
you return an error?
Konstantin

This function is similar to mlx4_check_rx_queue_offloads() in mlx4 driver.
As the git log message in the commit ce17eddefc20285bbfe575bdc07f42f0b20f34cb say
that a per port capability should has same setting (enabling or disabling) on both port
configuration via rte_eth_dev_configure( ) and queue configuration via rte_eth_rx_queue_setup( ).
This function check if this requirement is matched or not.
It also check offloading request is supported as a per port or a per queue capability or not.
If above checking is pass, it return 1 else return 0.

Ok, let be more specific here.
Let say:
requested == DEV_RX_OFFLOAD_VLAN_STRIP;
port_offloads == DEV_RX_OFFLOAD_IPV4_CKSUM;
port_supported = (DEV_RX_OFFLOAD_IPV4_CKSUM  |
		   DEV_RX_OFFLOAD_UDP_CKSUM   |
		   DEV_RX_OFFLOAD_TCP_CKSUM   |
		   DEV_RX_OFFLOAD_CRC_STRIP   |
		   DEV_RX_OFFLOAD_JUMBO_FRAME |
		   DEV_RX_OFFLOAD_SCATTER);

(port_offloads ^ requested) == DEV_RX_OFFLOAD_VLAN_STRIP | DEV_RX_OFFLOAD_IPV4_CKSUM;
(port_offloads ^ requested) & port_supported == DEV_RX_OFFLOAD_IPV4_CKSUM;
And that function will return failure, while as I understand it shouldn't - requested queue offload is valid.

Konstantin