Re: [dpdk-dev] [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API
Hi, Konstantin
Thanks for your patient guidance!
> 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.
>
Yes, rx function is related with offloading.
I'll remove this header split capability in my next patch set.
> >
> > > > +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
I'd like to list the git message of commit
ce17eddefc20285bbfe575bdc07f42f0b20f34cb which
has been submitted by Shahaf Shuler and already been accepted.
SHA-1: ce17eddefc20285bbfe575bdc07f42f0b20f34cb
* ethdev: introduce Rx queue offloads API
Introduce a new API to configure Rx offloads.
In the new API, offloads are divided into per-port and per-queue
offloads. The PMD reports capability for each of them.
Offloads are enabled using the existing DEV_RX_OFFLOAD_* flags.
To enable per-port offload, the offload should be set on both device
configuration and queue configuration. To enable per-queue offload, the
offloads can be set only on queue configuration.
Applications should set the ignore_offload_bitfield bit on rxmode
structure in order to move to the new API.
The old Rx offloads API is kept for the meanwhile, in order to enable a
smooth transition for PMDs and application to the new API.
Signed-off-by: Shahaf Shuler
Reviewed-by: Andrew Rybchenko
Acked-by: Konstantin Ananyev
In your example, IPV4_CKSUM is a per port offloading, it is
Enabled in port_offloads to rte_eth_dev_configure(), and it
Should also be enabled in requested to rte_eth_rx_queue_setup( ).
So your example fails in this checking.
This function is very similar with priv_is_rx_queue_offloads_allowed( ) in
/net/mlx5/mlx5_rxq.c
In the patch http://dpdk.org/dev/patchwork/patch/33386/ which has already been
accepted.
Re: [dpdk-dev] [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API
Hi Wei,
>
> Hi, Konstantin
> Thanks for your feedback.
>
> > -Original Message-
> > From: Ananyev, Konstantin
> > Sent: Thursday, March 15, 2018 5:48 AM
> > To: Dai, Wei ; Lu, Wenzhuo
> > Cc: dev@dpdk.org
> > Subject: RE: [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API
> >
> > Hi Wei,
> >
> > > -Original Message-
> > > From: Dai, Wei
> > > Sent: Wednesday, March 7, 2018 1:06 PM
> > > To: Lu, Wenzhuo ; Ananyev, Konstantin
> > >
> > > Cc: dev@dpdk.org; Dai, Wei
> > > 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
> > > ---
> > > drivers/net/ixgbe/ixgbe_ethdev.c | 93 +
> > > drivers/net/ixgbe/ixgbe_ipsec.c | 8 +-
> > > drivers/net/ixgbe/ixgbe_rxtx.c| 163
> > ++
> > > 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.
>
> > > +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
Re: [dpdk-dev] [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API
Hi, Konstantin
Thanks for your feedback.
> -Original Message-
> From: Ananyev, Konstantin
> Sent: Thursday, March 15, 2018 5:48 AM
> To: Dai, Wei ; Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API
>
> Hi Wei,
>
> > -Original Message-
> > From: Dai, Wei
> > Sent: Wednesday, March 7, 2018 1:06 PM
> > To: Lu, Wenzhuo ; Ananyev, Konstantin
> >
> > Cc: dev@dpdk.org; Dai, Wei
> > 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
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 93 +
> > drivers/net/ixgbe/ixgbe_ipsec.c | 8 +-
> > drivers/net/ixgbe/ixgbe_rxtx.c| 163
> ++
> > 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( ).
> > +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.
Re: [dpdk-dev] [PATCH v2 3/4] net/ixgbe: convert to new Rx offloads API
Hi Wei,
> -Original Message-
> From: Dai, Wei
> Sent: Wednesday, March 7, 2018 1:06 PM
> To: Lu, Wenzhuo ; Ananyev, Konstantin
>
> Cc: dev@dpdk.org; Dai, Wei
> 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
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 93 +
> drivers/net/ixgbe/ixgbe_ipsec.c | 8 +-
> drivers/net/ixgbe/ixgbe_rxtx.c| 163
> ++
> 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(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 8bb67ba..9437f05 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2105,19 +2105,22 @@ ixgbe_vlan_hw_strip_config(struct rte_eth_dev *dev)
> static int
> ixgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask)
> {
> + struct rte_eth_rxmode *rxmode;
> + rxmode = &dev->data->dev_conf.rxmode;
> +
> if (mask & ETH_VLAN_STRIP_MASK) {
> ixgbe_vlan_hw_strip_config(dev);
> }
>
> if (mask & ETH_VLAN_FILTER_MASK) {
> - if (dev->data->dev_conf.rxmode.hw_vlan_filter)
> + if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER)
> ixgbe_vlan_hw_filter_enable(dev);
> else
> ixgbe_vlan_hw_filter_disable(dev);
> }
>
> if (mask & ETH_VLAN_EXTEND_MASK) {
> - if (dev->data->dev_conf.rxmode.hw_vlan_extend)
> + if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> ixgbe_vlan_hw_extend_enable(dev);
> else
> ixgbe_vlan_hw_extend_disable(dev);
> @@ -2332,6 +2335,8 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
> IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> struct ixgbe_adapter *adapter =
> (struct ixgbe_adapter *)dev->data->dev_private;
> + struct rte_eth_dev_info dev_info;
> + uint64_t rx_offloads;
> int ret;
>
> PMD_INIT_FUNC_TRACE();
> @@ -2343,6 +2348,15 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
> return ret;
> }
>
> + ixgbe_dev_info_get(dev, &dev_info);
> + rx_offloads = dev->data->dev_conf.rxmode.offloads;
> + if ((rx_offloads & dev_info.rx_offload_capa) != rx_offloads) {
> + PMD_DRV_LOG(ERR, "Some Rx offloads are not supported "
> + "requested 0x%" PRIx64 " supported 0x%" PRIx64,
> + rx_offloads, dev_info.rx_offload_capa);
> + return -ENOTSUP;
> + }
> +
> /* set flag to update link status after init */
> intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE;
>
> @@ -3632,30 +3646,9 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
> else
> dev_info->max_vmdq_pools = ETH_64_POOLS;
> dev_info->vmdq_queue_num = dev_info->max_rx_queues;
> - dev_info->rx_offload_capa =
> - DEV_RX_OFFLOAD_VLAN_STRIP |
> - DEV_RX_OFFLOAD_IPV4_CKSUM |
> - DEV_RX_OFFLOAD_UDP_CKSUM |
> - DEV_RX_OFFLOAD_TCP_CKSUM |
> - DEV_RX_OFFLOAD_CRC_STRIP;
> -
> - /*
> - * RSC is only supported by 82599 and x540 PF devices in a non-SR-IOV
> - * mode.
> - */
> - if ((hw->mac.type == ixgbe_mac_82599EB ||
> - hw->mac.type == ixgbe_mac_X540) &&
> - !RTE_ETH_DEV_SRIOV(dev).active)
> - dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_TCP_LRO;
> -
> - if (hw->mac.type == ixgbe_mac_82599EB ||
> - hw->mac.type == ixgbe_mac_X540)
> - dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_MACSEC_STRIP;
> -
> - if (hw->mac.type == ixgbe_mac_X550 ||
> - hw->mac.type == ixgbe_mac_X550EM_x ||
> - hw->mac.type == ixgbe_mac_X550EM_a)
> - dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM;
> + dev_info->rx_queue_offload_capa = ixgbe_get_rx_queue_offloads(dev);
> + dev_info->rx_offload_capa = (ixgbe_get_rx_port_offloads(dev) |
> + dev_info->rx_queue_offload_capa);
>
> dev_info->tx_offload_capa =
> DEV_TX_OFFLOAD_VLAN_INSERT |
> @@ -3675,10 +3668,8 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
> dev_info->tx_offload_capa |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
>
> #ifdef RTE_LIBRTE_SECURITY
> - if (dev->security_ctx) {
> - dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_SECURITY;
> + if (dev->security_ctx)
> dev_info->tx_offload_capa |= DEV_TX_OFFLO
