[dpdk-dev,4/4] ethdev: add helpers to move to the new offloads API
Checks
Commit Message
A new offloads API was introduced by commits:
commit 121fff673172 ("ethdev: introduce Rx queue offloads API")
commit 35ac80d92f29 ("ethdev: introduce Tx queue offloads API")
In order to enable the PMDs to support only one of the APIs,
a conversion functions from the old to new API were added.
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
lib/librte_ether/rte_ethdev.c | 99 +++++++++++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 2 deletions(-)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shahaf Shuler
> Sent: Monday, September 4, 2017 8:12 AM
> To: thomas@monjalon.net
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> A new offloads API was introduced by commits:
>
> commit 121fff673172 ("ethdev: introduce Rx queue offloads API")
> commit 35ac80d92f29 ("ethdev: introduce Tx queue offloads API")
>
> In order to enable the PMDs to support only one of the APIs,
> a conversion functions from the old to new API were added.
>
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> lib/librte_ether/rte_ethdev.c | 99 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 50f8aa98d..1aa21a129 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1006,6 +1006,34 @@ rte_eth_dev_close(uint8_t port_id)
> dev->data->tx_queues = NULL;
> }
>
> +/**
> + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> + * offloads API.
> + */
> +static void
> +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> + struct rte_eth_rxq_conf *rxq_conf)
> +{
I think you need to:
rxq_conf->offloads = 0;
first here.
> + if (rxmode->header_split == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> + if (rxmode->hw_ip_checksum == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> + if (rxmode->hw_vlan_filter == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> + if (rxmode->hw_vlan_strip == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> + if (rxmode->hw_vlan_extend == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> + if (rxmode->jumbo_frame == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> + if (rxmode->hw_strip_crc == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> + if (rxmode->enable_scatter == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> + if (rxmode->enable_lro == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_TCP_LRO;
> +}
> +
> int
> rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> uint16_t nb_rx_desc, unsigned int socket_id,
> @@ -1016,6 +1044,8 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> uint32_t mbp_buf_size;
> struct rte_eth_dev *dev;
> struct rte_eth_dev_info dev_info;
> + struct rte_eth_rxq_conf rxq_trans_conf;
> + /* Holds translated configuration to be passed to the PMD */
> void **rxq;
>
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> @@ -1062,6 +1092,11 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> return -EINVAL;
> }
>
> + if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
> + (dev->data->dev_conf.rxmode.ignore_offloads == 1)) {
> + return -ENOTSUP;
> + }
> +
> if (nb_rx_desc > dev_info.rx_desc_lim.nb_max ||
> nb_rx_desc < dev_info.rx_desc_lim.nb_min ||
> nb_rx_desc % dev_info.rx_desc_lim.nb_align != 0) {
> @@ -1086,8 +1121,15 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> if (rx_conf == NULL)
> rx_conf = &dev_info.default_rxconf;
>
> + rxq_trans_conf = *rx_conf;
> + if ((dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD) &&
> + (dev->data->dev_conf.rxmode.ignore_offloads == 0)) {
> + rte_eth_convert_rxmode_offloads(&dev->data->dev_conf.rxmode,
> + &rxq_trans_conf);
> + }
> +
> ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
> - socket_id, rx_conf, mp);
> + socket_id, &rxq_trans_conf, mp);
> if (!ret) {
> if (!dev->data->min_rx_buf_size ||
> dev->data->min_rx_buf_size > mbp_buf_size)
> @@ -1097,6 +1139,49 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> return ret;
> }
>
> +/**
> + * A conversion function from txq_flags to rte_eth_txq_conf offloads API.
> + */
> +static void
> +rte_eth_convert_txq_flags(struct rte_eth_txq_conf *txq_conf)
> +{
> + uint32_t txq_flags = txq_conf->txq_flags;
> + uint64_t *offloads = &txq_conf->offloads;
I think you need to:
*offloads = 0;
first here.
BTW, might be a bit cleaner:
uint64_t offloads;
offloads = 0;
<conversion code>
txq_conf->tx_offloads = offloads;
Konstantin
> +
> + if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
> + *offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
> + if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
> + *offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
> + if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
> + *offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> + if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
> + *offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> + if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
> + *offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
> +}
> +
> +/**
> + * A conversion function between rte_eth_txq_conf offloads API to txq_flags
> + * offloads API.
> + */
> +static void
> +rte_eth_convert_txq_offloads(struct rte_eth_txq_conf *txq_conf)
> +{
> + uint32_t *txq_flags = &txq_conf->txq_flags;
> + uint64_t offloads = txq_conf->offloads;
> +
> + if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
> + *txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
> + if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
> + *txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
> + if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
> + *txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
> + if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
> + *txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
> + if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
> + *txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP;
> +}
> +
> int
> rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> uint16_t nb_tx_desc, unsigned int socket_id,
> @@ -1104,6 +1189,8 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> {
> struct rte_eth_dev *dev;
> struct rte_eth_dev_info dev_info;
> + struct rte_eth_txq_conf txq_trans_conf;
> + /* Holds translated configuration to be passed to the PMD */
> void **txq;
>
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> @@ -1148,8 +1235,16 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> if (tx_conf == NULL)
> tx_conf = &dev_info.default_txconf;
>
> + txq_trans_conf = *tx_conf;
> + if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> + (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
> + rte_eth_convert_txq_flags(&txq_trans_conf);
> + else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> + (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
> + rte_eth_convert_txq_offloads(&txq_trans_conf);
> +
> return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id, nb_tx_desc,
> - socket_id, tx_conf);
> + socket_id, &txq_trans_conf);
> }
>
> void
> --
> 2.12.0
Hi Shahaf,
> }
>
> +/**
> + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> + * offloads API.
> + */
> +static void
> +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> + struct rte_eth_rxq_conf *rxq_conf)
> +{
> + if (rxmode->header_split == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> + if (rxmode->hw_ip_checksum == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> + if (rxmode->hw_vlan_filter == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
Thinking on it a bit more:
VLAN_FILTER is definitely one per device, as it would affect VFs also.
At least that's what we have for Intel devices (ixgbe, i40e) right now.
For Intel devices VLAN_STRIP is also per device and
will also be applied to all corresponding VFs.
In fact, right now it is possible to query/change these 3 vlan offload flags on the fly
(after dev_start) on port basis by rte_eth_dev_(get|set)_vlan_offload API.
So, I think at least these 3 flags need to be remained on a port basis.
In fact, why can't we have both per port and per queue RX offload:
- dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port basis.
- rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on a queue basis.
- if particular RX_OFFLOAD flag for that device couldn't be setup on a queue basis -
rx_queue_setup() will return an error.
- rte_eth_rxq_info can be extended to provide information which RX_OFFLOADs
can be configured on a per queue basis.
BTW - in that case we probably wouldn't need ignore flag inside rx_conf anymore.
> + if (rxmode->hw_vlan_strip == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> + if (rxmode->hw_vlan_extend == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> + if (rxmode->jumbo_frame == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
There are some extra checks for that flag inside rte_eth_dev_configure().
If we going so support it per queue - then it probably need to be updated.
> + if (rxmode->hw_strip_crc == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> + if (rxmode->enable_scatter == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> + if (rxmode->enable_lro == 1)
> + rxq_conf->offloads |= DEV_RX_OFFLOAD_TCP_LRO;
> +}
> +
Konstantin
04/09/2017 15:25, Ananyev, Konstantin:
> Hi Shahaf,
>
> > +/**
> > + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> > + * offloads API.
> > + */
> > +static void
> > +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > + struct rte_eth_rxq_conf *rxq_conf)
> > +{
> > + if (rxmode->header_split == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > + if (rxmode->hw_ip_checksum == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > + if (rxmode->hw_vlan_filter == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
>
> Thinking on it a bit more:
> VLAN_FILTER is definitely one per device, as it would affect VFs also.
> At least that's what we have for Intel devices (ixgbe, i40e) right now.
> For Intel devices VLAN_STRIP is also per device and
> will also be applied to all corresponding VFs.
> In fact, right now it is possible to query/change these 3 vlan offload flags on the fly
> (after dev_start) on port basis by rte_eth_dev_(get|set)_vlan_offload API.
> So, I think at least these 3 flags need to be remained on a port basis.
I don't understand how it helps to be able to configure the same thing
in 2 places.
I think you are just describing a limitation of these HW: some offloads
must be the same for all queues.
It does not prevent from configuring them in the per-queue setup.
> In fact, why can't we have both per port and per queue RX offload:
> - dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port basis.
> - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on a queue basis.
> - if particular RX_OFFLOAD flag for that device couldn't be setup on a queue basis -
> rx_queue_setup() will return an error.
The queue setup can work while the value is the same for every queues.
> - rte_eth_rxq_info can be extended to provide information which RX_OFFLOADs
> can be configured on a per queue basis.
Yes the PMD should advertise its limitations like being forced to
apply the same configuration to all its queues.
> BTW - in that case we probably wouldn't need ignore flag inside rx_conf anymore.
Hi Konstantin,
Monday, September 4, 2017 4:25 PM, Ananyev, Konstantin:
>
> Hi Shahaf,
>
> > }
> >
> > +/**
> > + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> > + * offloads API.
> > + */
> > +static void
> > +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > + struct rte_eth_rxq_conf *rxq_conf) {
> > + if (rxmode->header_split == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > + if (rxmode->hw_ip_checksum == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > + if (rxmode->hw_vlan_filter == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
>
> Thinking on it a bit more:
> VLAN_FILTER is definitely one per device, as it would affect VFs also.
> At least that's what we have for Intel devices (ixgbe, i40e) right now.
This is vendor specific. For Mellanox this is one per device (regardless if it is a vf/pf).
> For Intel devices VLAN_STRIP is also per device and will also be applied to all
> corresponding VFs.
Again - vendor specific. For Mellanox is per queue.
> In fact, right now it is possible to query/change these 3 vlan offload flags on
> the fly (after dev_start) on port basis by
> rte_eth_dev_(get|set)_vlan_offload API.
> So, I think at least these 3 flags need to be remained on a port basis.
Am not sure I agree.
Why, for example, block from application the option to set some queues with vlan strip and some without if device allows?
Also how will we decide which offloads should stay per port and which are allowed to move per queue? this much depends on the underlying PMD.
Looks like i missed that part on ethdev, and if Rx offload will be per queue I will need to change it also.
> In fact, why can't we have both per port and per queue RX offload:
> - dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port
> basis.
> - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on
> a queue basis.
> - if particular RX_OFFLOAD flag for that device couldn't be setup on a queue
> basis -
> rx_queue_setup() will return an error.
Why not taking the per port configuration as a sub-case of per queue configuration?
For per-port offloads as long as the same configuration applies the queue setup succeeds.
> - rte_eth_rxq_info can be extended to provide information which
> RX_OFFLOADs
> can be configured on a per queue basis.
I am OK with the info suggestion.
> BTW - in that case we probably wouldn't need ignore flag inside rx_conf
> anymore.
>
>
> > + if (rxmode->hw_vlan_strip == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> > + if (rxmode->hw_vlan_extend == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> > + if (rxmode->jumbo_frame == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>
> There are some extra checks for that flag inside rte_eth_dev_configure().
> If we going so support it per queue - then it probably need to be updated.
>
> > + if (rxmode->hw_strip_crc == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> > + if (rxmode->enable_scatter == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> > + if (rxmode->enable_lro == 1)
> > + rxq_conf->offloads |= DEV_RX_OFFLOAD_TCP_LRO; }
> > +
>
> Konstantin
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, September 4, 2017 2:54 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> 04/09/2017 15:25, Ananyev, Konstantin:
> > Hi Shahaf,
> >
> > > +/**
> > > + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> > > + * offloads API.
> > > + */
> > > +static void
> > > +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > > + struct rte_eth_rxq_conf *rxq_conf)
> > > +{
> > > + if (rxmode->header_split == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > > + if (rxmode->hw_ip_checksum == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > > + if (rxmode->hw_vlan_filter == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> >
> > Thinking on it a bit more:
> > VLAN_FILTER is definitely one per device, as it would affect VFs also.
> > At least that's what we have for Intel devices (ixgbe, i40e) right now.
> > For Intel devices VLAN_STRIP is also per device and
> > will also be applied to all corresponding VFs.
> > In fact, right now it is possible to query/change these 3 vlan offload flags on the fly
> > (after dev_start) on port basis by rte_eth_dev_(get|set)_vlan_offload API.
> > So, I think at least these 3 flags need to be remained on a port basis.
>
> I don't understand how it helps to be able to configure the same thing
> in 2 places.
Because some offloads are per device, another - per queue.
Configuring on a device basis would allow most users to conjure all
queues in the same manner by default.
Those users who would need more fine-grained setup (per queue)
will be able to overwrite it by rx_queue_setup().
> I think you are just describing a limitation of these HW: some offloads
> must be the same for all queues.
As I said above - on some devices some offloads might also affect queues
that belong to VFs (to another ports in DPDK words).
You might never invoke rx_queue_setup() for these queues per your app.
But you still want to enable this offload on that device.
> It does not prevent from configuring them in the per-queue setup.
>
> > In fact, why can't we have both per port and per queue RX offload:
> > - dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port basis.
> > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on a queue basis.
> > - if particular RX_OFFLOAD flag for that device couldn't be setup on a queue basis -
> > rx_queue_setup() will return an error.
>
> The queue setup can work while the value is the same for every queues.
Ok, and how people would know that?
That for device N offload X has to be the same for all queues,
and for device M offload X can be differs for different queues.
Again, if we don't allow to enable/disable offloads for particular queue,
why to bother with updating rx_queue_setup() API at all?
>
> > - rte_eth_rxq_info can be extended to provide information which RX_OFFLOADs
> > can be configured on a per queue basis.
>
> Yes the PMD should advertise its limitations like being forced to
> apply the same configuration to all its queues.
Didn't get your last sentence.
Konstantin
>
> > BTW - in that case we probably wouldn't need ignore flag inside rx_conf anymore.
> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Monday, September 4, 2017 3:03 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> Hi Konstantin,
>
> Monday, September 4, 2017 4:25 PM, Ananyev, Konstantin:
> >
> > Hi Shahaf,
> >
> > > }
> > >
> > > +/**
> > > + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> > > + * offloads API.
> > > + */
> > > +static void
> > > +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > > + struct rte_eth_rxq_conf *rxq_conf) {
> > > + if (rxmode->header_split == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > > + if (rxmode->hw_ip_checksum == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > > + if (rxmode->hw_vlan_filter == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> >
> > Thinking on it a bit more:
> > VLAN_FILTER is definitely one per device, as it would affect VFs also.
> > At least that's what we have for Intel devices (ixgbe, i40e) right now.
>
> This is vendor specific. For Mellanox this is one per device (regardless if it is a vf/pf).
>
> > For Intel devices VLAN_STRIP is also per device and will also be applied to all
> > corresponding VFs.
>
> Again - vendor specific. For Mellanox is per queue.
Yep, I understand it varies quite a lot from vendor from vendor.
That's why I started to think we need to have to allow user to specify rx_offloads
for both port and queue basis.
>
> > In fact, right now it is possible to query/change these 3 vlan offload flags on
> > the fly (after dev_start) on port basis by
> > rte_eth_dev_(get|set)_vlan_offload API.
> > So, I think at least these 3 flags need to be remained on a port basis.
>
> Am not sure I agree.
> Why, for example, block from application the option to set some queues with vlan strip and some without if device allows?
> Also how will we decide which offloads should stay per port and which are allowed to move per queue? this much depends on the
> underlying PMD.
>
> Looks like i missed that part on ethdev, and if Rx offload will be per queue I will need to change it also.
>
>
> > In fact, why can't we have both per port and per queue RX offload:
> > - dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port
> > basis.
> > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on
> > a queue basis.
> > - if particular RX_OFFLOAD flag for that device couldn't be setup on a queue
> > basis -
> > rx_queue_setup() will return an error.
>
> Why not taking the per port configuration as a sub-case of per queue configuration?
> For per-port offloads as long as the same configuration applies the queue setup succeeds.
Do you mean that as long as queue config for offload X matches port config for the same offload - it is ok,
even if for that particular device offload X is per port not per queue?
Let say:
...
rxconf.offloads = DEV_RX_OFFLOAD_VLAN_STRIP;
...
rte_eth_dev_configure(port, ...);
...
/* queue offloads matches port offloads - always */
rte_eth_rx_queue_setup(port, ..., &rcxonf);
...
rxconf.offloads &= ~ DEV_RX_OFFLOAD_VLAN_STRIP;
/* ok for devices where vlan_stip per queue is supported,
* fails for devices with vlan_strip offload is per device.
*/
rte_eth_rx_queue_setup(port, ..., &rcxonf);
?
Konstantin
>
>
> > - rte_eth_rxq_info can be extended to provide information which
> > RX_OFFLOADs
> > can be configured on a per queue basis.
>
> I am OK with the info suggestion.
>
> > BTW - in that case we probably wouldn't need ignore flag inside rx_conf
> > anymore.
> >
> >
> > > + if (rxmode->hw_vlan_strip == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> > > + if (rxmode->hw_vlan_extend == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> > > + if (rxmode->jumbo_frame == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> >
> > There are some extra checks for that flag inside rte_eth_dev_configure().
> > If we going so support it per queue - then it probably need to be updated.
> >
> > > + if (rxmode->hw_strip_crc == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> > > + if (rxmode->enable_scatter == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> > > + if (rxmode->enable_lro == 1)
> > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_TCP_LRO; }
> > > +
> >
> > Konstantin
04/09/2017 16:18, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 04/09/2017 15:25, Ananyev, Konstantin:
> > > Hi Shahaf,
> > >
> > > > +/**
> > > > + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> > > > + * offloads API.
> > > > + */
> > > > +static void
> > > > +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > > > + struct rte_eth_rxq_conf *rxq_conf)
> > > > +{
> > > > + if (rxmode->header_split == 1)
> > > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > > > + if (rxmode->hw_ip_checksum == 1)
> > > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > > > + if (rxmode->hw_vlan_filter == 1)
> > > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> > >
> > > Thinking on it a bit more:
> > > VLAN_FILTER is definitely one per device, as it would affect VFs also.
> > > At least that's what we have for Intel devices (ixgbe, i40e) right now.
> > > For Intel devices VLAN_STRIP is also per device and
> > > will also be applied to all corresponding VFs.
> > > In fact, right now it is possible to query/change these 3 vlan offload flags on the fly
> > > (after dev_start) on port basis by rte_eth_dev_(get|set)_vlan_offload API.
> > > So, I think at least these 3 flags need to be remained on a port basis.
> >
> > I don't understand how it helps to be able to configure the same thing
> > in 2 places.
>
> Because some offloads are per device, another - per queue.
> Configuring on a device basis would allow most users to conjure all
> queues in the same manner by default.
> Those users who would need more fine-grained setup (per queue)
> will be able to overwrite it by rx_queue_setup().
Those users can set the same config for all queues.
>
> > I think you are just describing a limitation of these HW: some offloads
> > must be the same for all queues.
>
> As I said above - on some devices some offloads might also affect queues
> that belong to VFs (to another ports in DPDK words).
> You might never invoke rx_queue_setup() for these queues per your app.
> But you still want to enable this offload on that device.
You are advocating for per-port configuration API because
some settings must be the same on all the ports of your hardware?
So there is a big trouble. You don't need per-port settings,
but per-hw-device settings.
Or would you accept more fine-grained per-port settings?
If yes, you can accept even finer grained per-queues settings.
>
> > It does not prevent from configuring them in the per-queue setup.
> >
> > > In fact, why can't we have both per port and per queue RX offload:
> > > - dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port basis.
> > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on a queue basis.
> > > - if particular RX_OFFLOAD flag for that device couldn't be setup on a queue basis -
> > > rx_queue_setup() will return an error.
> >
> > The queue setup can work while the value is the same for every queues.
>
> Ok, and how people would know that?
> That for device N offload X has to be the same for all queues,
> and for device M offload X can be differs for different queues.
We can know the hardware limitations by filling this information
at PMD init.
> Again, if we don't allow to enable/disable offloads for particular queue,
> why to bother with updating rx_queue_setup() API at all?
I do not understand this question.
> > > - rte_eth_rxq_info can be extended to provide information which RX_OFFLOADs
> > > can be configured on a per queue basis.
> >
> > Yes the PMD should advertise its limitations like being forced to
> > apply the same configuration to all its queues.
>
> Didn't get your last sentence.
I agree that the hardware limitations must be written in an ethdev structure.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, September 5, 2017 8:48 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> 04/09/2017 16:18, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 04/09/2017 15:25, Ananyev, Konstantin:
> > > > Hi Shahaf,
> > > >
> > > > > +/**
> > > > > + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> > > > > + * offloads API.
> > > > > + */
> > > > > +static void
> > > > > +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > > > > + struct rte_eth_rxq_conf *rxq_conf)
> > > > > +{
> > > > > + if (rxmode->header_split == 1)
> > > > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > > > > + if (rxmode->hw_ip_checksum == 1)
> > > > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > > > > + if (rxmode->hw_vlan_filter == 1)
> > > > > + rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> > > >
> > > > Thinking on it a bit more:
> > > > VLAN_FILTER is definitely one per device, as it would affect VFs also.
> > > > At least that's what we have for Intel devices (ixgbe, i40e) right now.
> > > > For Intel devices VLAN_STRIP is also per device and
> > > > will also be applied to all corresponding VFs.
> > > > In fact, right now it is possible to query/change these 3 vlan offload flags on the fly
> > > > (after dev_start) on port basis by rte_eth_dev_(get|set)_vlan_offload API.
> > > > So, I think at least these 3 flags need to be remained on a port basis.
> > >
> > > I don't understand how it helps to be able to configure the same thing
> > > in 2 places.
> >
> > Because some offloads are per device, another - per queue.
> > Configuring on a device basis would allow most users to conjure all
> > queues in the same manner by default.
> > Those users who would need more fine-grained setup (per queue)
> > will be able to overwrite it by rx_queue_setup().
>
> Those users can set the same config for all queues.
> >
> > > I think you are just describing a limitation of these HW: some offloads
> > > must be the same for all queues.
> >
> > As I said above - on some devices some offloads might also affect queues
> > that belong to VFs (to another ports in DPDK words).
> > You might never invoke rx_queue_setup() for these queues per your app.
> > But you still want to enable this offload on that device.
I am ok with having per-port and per-queue offload configuration.
My concern is that after that patch only per-queue offload configuration will remain.
I think we need both.
Konstantin
>
> You are advocating for per-port configuration API because
> some settings must be the same on all the ports of your hardware?
> So there is a big trouble. You don't need per-port settings,
> but per-hw-device settings.
> Or would you accept more fine-grained per-port settings?
> If yes, you can accept even finer grained per-queues settings.
> >
> > > It does not prevent from configuring them in the per-queue setup.
> > >
> > > > In fact, why can't we have both per port and per queue RX offload:
> > > > - dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port basis.
> > > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on a queue basis.
> > > > - if particular RX_OFFLOAD flag for that device couldn't be setup on a queue basis -
> > > > rx_queue_setup() will return an error.
> > >
> > > The queue setup can work while the value is the same for every queues.
> >
> > Ok, and how people would know that?
> > That for device N offload X has to be the same for all queues,
> > and for device M offload X can be differs for different queues.
>
> We can know the hardware limitations by filling this information
> at PMD init.
>
> > Again, if we don't allow to enable/disable offloads for particular queue,
> > why to bother with updating rx_queue_setup() API at all?
>
> I do not understand this question.
>
> > > > - rte_eth_rxq_info can be extended to provide information which RX_OFFLOADs
> > > > can be configured on a per queue basis.
> > >
> > > Yes the PMD should advertise its limitations like being forced to
> > > apply the same configuration to all its queues.
> >
> > Didn't get your last sentence.
>
> I agree that the hardware limitations must be written in an ethdev structure.
Tuesday, September 5, 2017 11:10 AM, Ananyev, Konstantin:
> > > > > In fact, right now it is possible to query/change these 3 vlan
> > > > > offload flags on the fly (after dev_start) on port basis by
> rte_eth_dev_(get|set)_vlan_offload API.
Regarding this API from ethdev.
So this seems like a hack on ethdev. Currently there are 2 ways for user to set Rx vlan offloads.
One is through dev_configure which require the ports to be stopped. The other is this API which can set even if the port is started.
We should have only one place were application set offloads and this is currently on dev_configure,
And future to be on rx_queue_setup.
I would say that this API should be removed as well.
Application which wants to change those offloads will stop the ports and reconfigure the PMD.
Am quite sure that there are PMDs which need to re-create the Rxq based on vlan offloads changing and this cannot be done while the traffic flows.
> > > > > So, I think at least these 3 flags need to be remained on a port basis.
> > > >
> > > > I don't understand how it helps to be able to configure the same
> > > > thing in 2 places.
> > >
> > > Because some offloads are per device, another - per queue.
> > > Configuring on a device basis would allow most users to conjure all
> > > queues in the same manner by default.
> > > Those users who would need more fine-grained setup (per queue) will
> > > be able to overwrite it by rx_queue_setup().
> >
> > Those users can set the same config for all queues.
> > >
> > > > I think you are just describing a limitation of these HW: some
> > > > offloads must be the same for all queues.
> > >
> > > As I said above - on some devices some offloads might also affect
> > > queues that belong to VFs (to another ports in DPDK words).
> > > You might never invoke rx_queue_setup() for these queues per your
> app.
> > > But you still want to enable this offload on that device.
>
> I am ok with having per-port and per-queue offload configuration.
> My concern is that after that patch only per-queue offload configuration will
> remain.
> I think we need both.
So looks like we all agree PMDs should report as part of the rte_eth_dev_info_get which offloads are per port and which are per queue.
Regarding the offloads configuration by application I see 2 options:
1. have an API to set offloads per port as part of device configure and API to set offloads per queue as part of queue setup
2. set all offloads as part of queue configuration (per port offloads will be set equally for all queues). In case of a mixed configuration for port offloads PMD will return error.
Such error can be reported on device start. The PMD will traverse the queues and check for conflicts.
I will focus on the cons, since both achieve the goal:
Cons of #1:
- Two places to configure offloads.
- Like Thomas mentioned - what about offloads per device? This direction leads to more places to configure the offloads.
Cons of #2:
- Late error reporting - on device start and not on queue setup.
I would go with #2.
> Konstantin
>
> >
> > You are advocating for per-port configuration API because some
> > settings must be the same on all the ports of your hardware?
> > So there is a big trouble. You don't need per-port settings, but
> > per-hw-device settings.
> > Or would you accept more fine-grained per-port settings?
> > If yes, you can accept even finer grained per-queues settings.
> > >
> > > > It does not prevent from configuring them in the per-queue setup.
> > > >
> > > > > In fact, why can't we have both per port and per queue RX offload:
> > > > > - dev_configure() will accept RX_OFFLOAD_* flags and apply them on
> a port basis.
> > > > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply
> them on a queue basis.
> > > > > - if particular RX_OFFLOAD flag for that device couldn't be setup on a
> queue basis -
> > > > > rx_queue_setup() will return an error.
> > > >
> > > > The queue setup can work while the value is the same for every
> queues.
> > >
> > > Ok, and how people would know that?
> > > That for device N offload X has to be the same for all queues, and
> > > for device M offload X can be differs for different queues.
> >
> > We can know the hardware limitations by filling this information at
> > PMD init.
> >
> > > Again, if we don't allow to enable/disable offloads for particular
> > > queue, why to bother with updating rx_queue_setup() API at all?
> >
> > I do not understand this question.
> >
> > > > > - rte_eth_rxq_info can be extended to provide information which
> RX_OFFLOADs
> > > > > can be configured on a per queue basis.
> > > >
> > > > Yes the PMD should advertise its limitations like being forced to
> > > > apply the same configuration to all its queues.
> > >
> > > Didn't get your last sentence.
> >
> > I agree that the hardware limitations must be written in an ethdev
> structure.
05/09/2017 12:51, Shahaf Shuler:
> So looks like we all agree PMDs should report as part of the rte_eth_dev_info_get which offloads are per port and which are per queue.
>
> Regarding the offloads configuration by application I see 2 options:
> 1. have an API to set offloads per port as part of device configure and API to set offloads per queue as part of queue setup
> 2. set all offloads as part of queue configuration (per port offloads will be set equally for all queues). In case of a mixed configuration for port offloads PMD will return error.
> Such error can be reported on device start. The PMD will traverse the queues and check for conflicts.
>
> I will focus on the cons, since both achieve the goal:
>
> Cons of #1:
> - Two places to configure offloads.
> - Like Thomas mentioned - what about offloads per device? This direction leads to more places to configure the offloads.
>
> Cons of #2:
> - Late error reporting - on device start and not on queue setup.
Why not reporting error on queue setup?
> I would go with #2.
I vote also for #2
>
> > > > > > In fact, right now it is possible to query/change these 3 vlan
> > > > > > offload flags on the fly (after dev_start) on port basis by
> > rte_eth_dev_(get|set)_vlan_offload API.
>
> Regarding this API from ethdev.
>
> So this seems like a hack on ethdev. Currently there are 2 ways for user to set Rx vlan offloads.
> One is through dev_configure which require the ports to be stopped. The other is this API which can set even if the port is started.
Yes there is an ability to enable/disable VLAN offloads without stop/reconfigure the device.
Though I wouldn't call it 'a hack'.
From my perspective - it is a useful feature.
Same as it is possible in some cases to change MTU without stopping device, etc.
>
> We should have only one place were application set offloads and this is currently on dev_configure,
Hmm, if HW supports the ability to do things at runtime why we have to stop users from using that ability?
> And future to be on rx_queue_setup.
>
> I would say that this API should be removed as well.
> Application which wants to change those offloads will stop the ports and reconfigure the PMD.
I wouldn't agree - see above.
> Am quite sure that there are PMDs which need to re-create the Rxq based on vlan offloads changing and this cannot be done while the
> traffic flows.
That's an optional API - PMD can choose does it want to support it or not.
>
>
> > > > > > So, I think at least these 3 flags need to be remained on a port basis.
> > > > >
> > > > > I don't understand how it helps to be able to configure the same
> > > > > thing in 2 places.
> > > >
> > > > Because some offloads are per device, another - per queue.
> > > > Configuring on a device basis would allow most users to conjure all
> > > > queues in the same manner by default.
> > > > Those users who would need more fine-grained setup (per queue) will
> > > > be able to overwrite it by rx_queue_setup().
> > >
> > > Those users can set the same config for all queues.
> > > >
> > > > > I think you are just describing a limitation of these HW: some
> > > > > offloads must be the same for all queues.
> > > >
> > > > As I said above - on some devices some offloads might also affect
> > > > queues that belong to VFs (to another ports in DPDK words).
> > > > You might never invoke rx_queue_setup() for these queues per your
> > app.
> > > > But you still want to enable this offload on that device.
> >
> > I am ok with having per-port and per-queue offload configuration.
> > My concern is that after that patch only per-queue offload configuration will
> > remain.
> > I think we need both.
>
> So looks like we all agree PMDs should report as part of the rte_eth_dev_info_get which offloads are per port and which are per queue.
Yep.
>
> Regarding the offloads configuration by application I see 2 options:
> 1. have an API to set offloads per port as part of device configure and API to set offloads per queue as part of queue setup
> 2. set all offloads as part of queue configuration (per port offloads will be set equally for all queues). In case of a mixed configuration for
> port offloads PMD will return error.
> Such error can be reported on device start. The PMD will traverse the queues and check for conflicts.
>
> I will focus on the cons, since both achieve the goal:
>
> Cons of #1:
> - Two places to configure offloads.
Yes, but why is that a problem?
> - Like Thomas mentioned - what about offloads per device? This direction leads to more places to configure the offloads.
As you said above - there would be 2 places: per port and per queue.
Could you explain - what other places you are talking about?
>
> Cons of #2:
> - Late error reporting - on device start and not on queue setup.
Consider scenario when PF has a corresponding VFs
(PF is controlled by DPDK)
Right now (at least with Intel HW) it is possible to:
struct rte_eth_conf dev_conf;
dev_conf. rxmode.hw_vlan_filter = 1;
...
rte_eth_dev_configure(pf_port_id, 0, 0, &dev_conf);
rte_eth_dev_start(pf_port_id);
In that scenario I don't have any RX/TX queues configured.
Though I still able to enable vlan filter, and it would work correctly for VFs.
Same for other per-port offloads.
With approach #2 it simply wouldn't work.
So my preference is still #1.
Konstantin
>
> I would go with #2.
>
> > Konstantin
> >
> > >
> > > You are advocating for per-port configuration API because some
> > > settings must be the same on all the ports of your hardware?
> > > So there is a big trouble. You don't need per-port settings, but
> > > per-hw-device settings.
> > > Or would you accept more fine-grained per-port settings?
> > > If yes, you can accept even finer grained per-queues settings.
> > > >
> > > > > It does not prevent from configuring them in the per-queue setup.
> > > > >
> > > > > > In fact, why can't we have both per port and per queue RX offload:
> > > > > > - dev_configure() will accept RX_OFFLOAD_* flags and apply them on
> > a port basis.
> > > > > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply
> > them on a queue basis.
> > > > > > - if particular RX_OFFLOAD flag for that device couldn't be setup on a
> > queue basis -
> > > > > > rx_queue_setup() will return an error.
> > > > >
> > > > > The queue setup can work while the value is the same for every
> > queues.
> > > >
> > > > Ok, and how people would know that?
> > > > That for device N offload X has to be the same for all queues, and
> > > > for device M offload X can be differs for different queues.
> > >
> > > We can know the hardware limitations by filling this information at
> > > PMD init.
> > >
> > > > Again, if we don't allow to enable/disable offloads for particular
> > > > queue, why to bother with updating rx_queue_setup() API at all?
> > >
> > > I do not understand this question.
> > >
> > > > > > - rte_eth_rxq_info can be extended to provide information which
> > RX_OFFLOADs
> > > > > > can be configured on a per queue basis.
> > > > >
> > > > > Yes the PMD should advertise its limitations like being forced to
> > > > > apply the same configuration to all its queues.
> > > >
> > > > Didn't get your last sentence.
> > >
> > > I agree that the hardware limitations must be written in an ethdev
> > structure.
Tuesday, September 5, 2017 6:31 PM, Ananyev, Konstantin:
> >
> > > > > > > In fact, right now it is possible to query/change these 3
> > > > > > > vlan offload flags on the fly (after dev_start) on port
> > > > > > > basis by
> > > rte_eth_dev_(get|set)_vlan_offload API.
> >
> > Regarding this API from ethdev.
> >
> > So this seems like a hack on ethdev. Currently there are 2 ways for user to
> set Rx vlan offloads.
> > One is through dev_configure which require the ports to be stopped. The
> other is this API which can set even if the port is started.
>
> Yes there is an ability to enable/disable VLAN offloads without
> stop/reconfigure the device.
> Though I wouldn't call it 'a hack'.
> From my perspective - it is a useful feature.
> Same as it is possible in some cases to change MTU without stopping device,
> etc.
>
> >
> > We should have only one place were application set offloads and this
> > is currently on dev_configure,
>
> Hmm, if HW supports the ability to do things at runtime why we have to stop
> users from using that ability?
>
> > And future to be on rx_queue_setup.
> >
> > I would say that this API should be removed as well.
> > Application which wants to change those offloads will stop the ports and
> reconfigure the PMD.
>
> I wouldn't agree - see above.
>
> > Am quite sure that there are PMDs which need to re-create the Rxq
> > based on vlan offloads changing and this cannot be done while the traffic
> flows.
>
> That's an optional API - PMD can choose does it want to support it or not.
>
> >
> >
> > > > > > > So, I think at least these 3 flags need to be remained on a port
> basis.
> > > > > >
> > > > > > I don't understand how it helps to be able to configure the
> > > > > > same thing in 2 places.
> > > > >
> > > > > Because some offloads are per device, another - per queue.
> > > > > Configuring on a device basis would allow most users to conjure
> > > > > all queues in the same manner by default.
> > > > > Those users who would need more fine-grained setup (per queue)
> > > > > will be able to overwrite it by rx_queue_setup().
> > > >
> > > > Those users can set the same config for all queues.
> > > > >
> > > > > > I think you are just describing a limitation of these HW: some
> > > > > > offloads must be the same for all queues.
> > > > >
> > > > > As I said above - on some devices some offloads might also
> > > > > affect queues that belong to VFs (to another ports in DPDK words).
> > > > > You might never invoke rx_queue_setup() for these queues per
> > > > > your
> > > app.
> > > > > But you still want to enable this offload on that device.
> > >
> > > I am ok with having per-port and per-queue offload configuration.
> > > My concern is that after that patch only per-queue offload
> > > configuration will remain.
> > > I think we need both.
> >
> > So looks like we all agree PMDs should report as part of the
> rte_eth_dev_info_get which offloads are per port and which are per queue.
>
> Yep.
>
> >
> > Regarding the offloads configuration by application I see 2 options:
> > 1. have an API to set offloads per port as part of device configure
> > and API to set offloads per queue as part of queue setup 2. set all
> > offloads as part of queue configuration (per port offloads will be set equally
> for all queues). In case of a mixed configuration for port offloads PMD will
> return error.
> > Such error can be reported on device start. The PMD will traverse the
> queues and check for conflicts.
> >
> > I will focus on the cons, since both achieve the goal:
> >
> > Cons of #1:
> > - Two places to configure offloads.
>
> Yes, but why is that a problem?
If we could make the offloads API to set the offloads in a single place it would be much cleaner and less error prune.
There is one flow which change the offloads configuration.
Later on when we want to change/expend it will be much simpler, as all modification can happen in a single place only.
>
> > - Like Thomas mentioned - what about offloads per device? This direction
> leads to more places to configure the offloads.
>
> As you said above - there would be 2 places: per port and per queue.
> Could you explain - what other places you are talking about?
In fact, the vlan filter offload for PF is a *per device* offload and not per port. Since the corresponding VF has it just by the fact the PF set it on dev_configure.
So to be exact, such offload should be set on a new offload section called "per device offloads".
Currently you compromise on setting it in the *per port* offload section, with proper explanation on the VF limitation in intel.
>
> >
> > Cons of #2:
> > - Late error reporting - on device start and not on queue setup.
>
> Consider scenario when PF has a corresponding VFs (PF is controlled by
> DPDK) Right now (at least with Intel HW) it is possible to:
>
> struct rte_eth_conf dev_conf;
> dev_conf. rxmode.hw_vlan_filter = 1;
> ...
> rte_eth_dev_configure(pf_port_id, 0, 0, &dev_conf);
> rte_eth_dev_start(pf_port_id);
>
> In that scenario I don't have any RX/TX queues configured.
> Though I still able to enable vlan filter, and it would work correctly for VFs.
> Same for other per-port offloads.
For the PF - enabling vlan filtering without any queues means nothing. The PF can receive no traffic, what different does it makes the vlan filtering is set?
For the VF - I assume it will have queues, therefore for it vlan filtering has a meaning. However as I said above, the VF has the vlan filter because in intel this is per-device offload, so this is not a good example.
Which other per-port offloads you refer to?
I don't understand what is the meaning of setting per-port offloads without opening any Tx/Rx queues.
> With approach #2 it simply wouldn't work.
Yes for vlan filtering it will not work on intel, and this may be enough to move to suggestion #1.
Thomas?
>
> So my preference is still #1.
>
> Konstantin
>
> >
> > I would go with #2.
> >
> > > Konstantin
> > >
> > > >
> > > > You are advocating for per-port configuration API because some
> > > > settings must be the same on all the ports of your hardware?
> > > > So there is a big trouble. You don't need per-port settings, but
> > > > per-hw-device settings.
> > > > Or would you accept more fine-grained per-port settings?
> > > > If yes, you can accept even finer grained per-queues settings.
> > > > >
> > > > > > It does not prevent from configuring them in the per-queue setup.
> > > > > >
> > > > > > > In fact, why can't we have both per port and per queue RX
> offload:
> > > > > > > - dev_configure() will accept RX_OFFLOAD_* flags and apply
> > > > > > > them on
> > > a port basis.
> > > > > > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and
> > > > > > > apply
> > > them on a queue basis.
> > > > > > > - if particular RX_OFFLOAD flag for that device couldn't be
> > > > > > > setup on a
> > > queue basis -
> > > > > > > rx_queue_setup() will return an error.
> > > > > >
> > > > > > The queue setup can work while the value is the same for every
> > > queues.
> > > > >
> > > > > Ok, and how people would know that?
> > > > > That for device N offload X has to be the same for all queues,
> > > > > and for device M offload X can be differs for different queues.
> > > >
> > > > We can know the hardware limitations by filling this information
> > > > at PMD init.
> > > >
> > > > > Again, if we don't allow to enable/disable offloads for
> > > > > particular queue, why to bother with updating rx_queue_setup() API
> at all?
> > > >
> > > > I do not understand this question.
> > > >
> > > > > > > - rte_eth_rxq_info can be extended to provide information
> > > > > > > which
> > > RX_OFFLOADs
> > > > > > > can be configured on a per queue basis.
> > > > > >
> > > > > > Yes the PMD should advertise its limitations like being forced
> > > > > > to apply the same configuration to all its queues.
> > > > >
> > > > > Didn't get your last sentence.
> > > >
> > > > I agree that the hardware limitations must be written in an ethdev
> > > structure.
> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Wednesday, September 6, 2017 7:02 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> Tuesday, September 5, 2017 6:31 PM, Ananyev, Konstantin:
> > >
> > > > > > > > In fact, right now it is possible to query/change these 3
> > > > > > > > vlan offload flags on the fly (after dev_start) on port
> > > > > > > > basis by
> > > > rte_eth_dev_(get|set)_vlan_offload API.
> > >
> > > Regarding this API from ethdev.
> > >
> > > So this seems like a hack on ethdev. Currently there are 2 ways for user to
> > set Rx vlan offloads.
> > > One is through dev_configure which require the ports to be stopped. The
> > other is this API which can set even if the port is started.
> >
> > Yes there is an ability to enable/disable VLAN offloads without
> > stop/reconfigure the device.
> > Though I wouldn't call it 'a hack'.
> > From my perspective - it is a useful feature.
> > Same as it is possible in some cases to change MTU without stopping device,
> > etc.
> >
> > >
> > > We should have only one place were application set offloads and this
> > > is currently on dev_configure,
> >
> > Hmm, if HW supports the ability to do things at runtime why we have to stop
> > users from using that ability?
> >
> > > And future to be on rx_queue_setup.
> > >
> > > I would say that this API should be removed as well.
> > > Application which wants to change those offloads will stop the ports and
> > reconfigure the PMD.
> >
> > I wouldn't agree - see above.
> >
> > > Am quite sure that there are PMDs which need to re-create the Rxq
> > > based on vlan offloads changing and this cannot be done while the traffic
> > flows.
> >
> > That's an optional API - PMD can choose does it want to support it or not.
> >
> > >
> > >
> > > > > > > > So, I think at least these 3 flags need to be remained on a port
> > basis.
> > > > > > >
> > > > > > > I don't understand how it helps to be able to configure the
> > > > > > > same thing in 2 places.
> > > > > >
> > > > > > Because some offloads are per device, another - per queue.
> > > > > > Configuring on a device basis would allow most users to conjure
> > > > > > all queues in the same manner by default.
> > > > > > Those users who would need more fine-grained setup (per queue)
> > > > > > will be able to overwrite it by rx_queue_setup().
> > > > >
> > > > > Those users can set the same config for all queues.
> > > > > >
> > > > > > > I think you are just describing a limitation of these HW: some
> > > > > > > offloads must be the same for all queues.
> > > > > >
> > > > > > As I said above - on some devices some offloads might also
> > > > > > affect queues that belong to VFs (to another ports in DPDK words).
> > > > > > You might never invoke rx_queue_setup() for these queues per
> > > > > > your
> > > > app.
> > > > > > But you still want to enable this offload on that device.
> > > >
> > > > I am ok with having per-port and per-queue offload configuration.
> > > > My concern is that after that patch only per-queue offload
> > > > configuration will remain.
> > > > I think we need both.
> > >
> > > So looks like we all agree PMDs should report as part of the
> > rte_eth_dev_info_get which offloads are per port and which are per queue.
> >
> > Yep.
> >
> > >
> > > Regarding the offloads configuration by application I see 2 options:
> > > 1. have an API to set offloads per port as part of device configure
> > > and API to set offloads per queue as part of queue setup 2. set all
> > > offloads as part of queue configuration (per port offloads will be set equally
> > for all queues). In case of a mixed configuration for port offloads PMD will
> > return error.
> > > Such error can be reported on device start. The PMD will traverse the
> > queues and check for conflicts.
> > >
> > > I will focus on the cons, since both achieve the goal:
> > >
> > > Cons of #1:
> > > - Two places to configure offloads.
> >
> > Yes, but why is that a problem?
>
> If we could make the offloads API to set the offloads in a single place it would be much cleaner and less error prune.
> There is one flow which change the offloads configuration.
> Later on when we want to change/expend it will be much simpler, as all modification can happen in a single place only.
Ok I understand that intention, but I don't think it would fit for all cases.
From my perspective it is not that big hassle to specify offloads for per-port and per-queue way.
Again we still have offloads that could be enabled/disabled without device/queue stop.
>
> >
> > > - Like Thomas mentioned - what about offloads per device? This direction
> > leads to more places to configure the offloads.
> >
> > As you said above - there would be 2 places: per port and per queue.
> > Could you explain - what other places you are talking about?
>
> In fact, the vlan filter offload for PF is a *per device* offload and not per port. Since the corresponding VF has it just by the fact the PF set it
> on dev_configure.
I don't understand why you differ per-device and per-port offloads.
As I remember, right now there is one to one mapping between ethdev and portid inside DPDK.
All rte_ethdev functions do refer device through port id.
We can name it per-device or per-port offloads - whatever you like - it wouldn't change anything.
> So to be exact, such offload should be set on a new offload section called "per device offloads".
> Currently you compromise on setting it in the *per port* offload section, with proper explanation on the VF limitation in intel.
>
> >
> > >
> > > Cons of #2:
> > > - Late error reporting - on device start and not on queue setup.
> >
> > Consider scenario when PF has a corresponding VFs (PF is controlled by
> > DPDK) Right now (at least with Intel HW) it is possible to:
> >
> > struct rte_eth_conf dev_conf;
> > dev_conf. rxmode.hw_vlan_filter = 1;
> > ...
> > rte_eth_dev_configure(pf_port_id, 0, 0, &dev_conf);
> > rte_eth_dev_start(pf_port_id);
> >
> > In that scenario I don't have any RX/TX queues configured.
> > Though I still able to enable vlan filter, and it would work correctly for VFs.
> > Same for other per-port offloads.
>
> For the PF - enabling vlan filtering without any queues means nothing. The PF can receive no traffic, what different does it makes the vlan
> filtering is set?
> For the VF - I assume it will have queues, therefore for it vlan filtering has a meaning. However as I said above, the VF has the vlan filter
> because in intel this is per-device offload, so this is not a good example.
Yes it is a per-device offload, and right now it is possible to enable/disable it via
dev_confgiure(); dev_start();
without configuring/starting any RX/TX queues.
That's an ability I'd like to preserve.
So from my perspective it is a perfectly valid example.
Konstantin
>
> Which other per-port offloads you refer to?
> I don't understand what is the meaning of setting per-port offloads without opening any Tx/Rx queues.
>
>
> > With approach #2 it simply wouldn't work.
>
> Yes for vlan filtering it will not work on intel, and this may be enough to move to suggestion #1.
>
> Thomas?
>
> >
> > So my preference is still #1.
> >
> > Konstantin
> >
> > >
> > > I would go with #2.
> > >
> > > > Konstantin
> > > >
> > > > >
> > > > > You are advocating for per-port configuration API because some
> > > > > settings must be the same on all the ports of your hardware?
> > > > > So there is a big trouble. You don't need per-port settings, but
> > > > > per-hw-device settings.
> > > > > Or would you accept more fine-grained per-port settings?
> > > > > If yes, you can accept even finer grained per-queues settings.
> > > > > >
> > > > > > > It does not prevent from configuring them in the per-queue setup.
> > > > > > >
> > > > > > > > In fact, why can't we have both per port and per queue RX
> > offload:
> > > > > > > > - dev_configure() will accept RX_OFFLOAD_* flags and apply
> > > > > > > > them on
> > > > a port basis.
> > > > > > > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and
> > > > > > > > apply
> > > > them on a queue basis.
> > > > > > > > - if particular RX_OFFLOAD flag for that device couldn't be
> > > > > > > > setup on a
> > > > queue basis -
> > > > > > > > rx_queue_setup() will return an error.
> > > > > > >
> > > > > > > The queue setup can work while the value is the same for every
> > > > queues.
> > > > > >
> > > > > > Ok, and how people would know that?
> > > > > > That for device N offload X has to be the same for all queues,
> > > > > > and for device M offload X can be differs for different queues.
> > > > >
> > > > > We can know the hardware limitations by filling this information
> > > > > at PMD init.
> > > > >
> > > > > > Again, if we don't allow to enable/disable offloads for
> > > > > > particular queue, why to bother with updating rx_queue_setup() API
> > at all?
> > > > >
> > > > > I do not understand this question.
> > > > >
> > > > > > > > - rte_eth_rxq_info can be extended to provide information
> > > > > > > > which
> > > > RX_OFFLOADs
> > > > > > > > can be configured on a per queue basis.
> > > > > > >
> > > > > > > Yes the PMD should advertise its limitations like being forced
> > > > > > > to apply the same configuration to all its queues.
> > > > > >
> > > > > > Didn't get your last sentence.
> > > > >
> > > > > I agree that the hardware limitations must be written in an ethdev
> > > > structure.
I still think we must streamline ethdev API instead of complexifying.
We should drop the big "configure everything" and configure offloads
one by one, and per queue (the finer grain).
More comments below
06/09/2017 11:33, Ananyev, Konstantin:
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> > Tuesday, September 5, 2017 6:31 PM, Ananyev, Konstantin:
> > > > > > > > > In fact, right now it is possible to query/change these 3
> > > > > > > > > vlan offload flags on the fly (after dev_start) on port
> > > > > > > > > basis by
> > > > > rte_eth_dev_(get|set)_vlan_offload API.
> > > >
> > > > Regarding this API from ethdev.
> > > >
> > > > So this seems like a hack on ethdev. Currently there are 2 ways for user to
> > > set Rx vlan offloads.
> > > > One is through dev_configure which require the ports to be stopped. The
> > > other is this API which can set even if the port is started.
> > >
> > > Yes there is an ability to enable/disable VLAN offloads without
> > > stop/reconfigure the device.
> > > Though I wouldn't call it 'a hack'.
> > > From my perspective - it is a useful feature.
> > > Same as it is possible in some cases to change MTU without stopping device,
> > > etc.
I think the function rte_eth_dev_configure(), which set up several things
at a time, is a very bad idea from API perspective.
In the VLAN example, we should have only one function to set this offload.
And the PMD should advertise the capability of configuring VLAN on the fly.
This function should return an error if called on the fly (started state)
and PMD does not support it.
> > > Consider scenario when PF has a corresponding VFs (PF is controlled by
> > > DPDK) Right now (at least with Intel HW) it is possible to:
> > >
> > > struct rte_eth_conf dev_conf;
> > > dev_conf. rxmode.hw_vlan_filter = 1;
> > > ...
> > > rte_eth_dev_configure(pf_port_id, 0, 0, &dev_conf);
> > > rte_eth_dev_start(pf_port_id);
> > >
> > > In that scenario I don't have any RX/TX queues configured.
> > > Though I still able to enable vlan filter, and it would work correctly for VFs.
> > > Same for other per-port offloads.
> >
> > For the PF - enabling vlan filtering without any queues means nothing. The PF can receive no traffic, what different does it makes the vlan
> > filtering is set?
> > For the VF - I assume it will have queues, therefore for it vlan filtering has a meaning. However as I said above, the VF has the vlan filter
> > because in intel this is per-device offload, so this is not a good example.
>
> Yes it is a per-device offload, and right now it is possible to enable/disable it via
> dev_confgiure(); dev_start();
> without configuring/starting any RX/TX queues.
> That's an ability I'd like to preserve.
> So from my perspective it is a perfectly valid example.
It is configuring VFs by setting the PF.
Where is it documented?
It looks to me as a device-specific side effect.
Wednesday, September 13, 2017 12:28 PM, Thomas Monjalon:
> I still think we must streamline ethdev API instead of complexifying.
> We should drop the big "configure everything" and configure offloads one by
> one, and per queue (the finer grain).
The issue is, that there is some functionality which cannot be achieved when configuring offload per queue.
For example - vlan filter on intel NICs. The PF can set it even without creating a single queue, in order to enable it for the VFs.
To make it right, such functionality resides on per-device offloads. However we cannot have such concept on ethdev layer.
Also not sure we want to modify the eal for that.
>
> More comments below
>
> 06/09/2017 11:33, Ananyev, Konstantin:
> > From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> > > Tuesday, September 5, 2017 6:31 PM, Ananyev, Konstantin:
>
> > > > > > > > > > In fact, right now it is possible to query/change
> > > > > > > > > > these 3 vlan offload flags on the fly (after
> > > > > > > > > > dev_start) on port basis by
> > > > > > rte_eth_dev_(get|set)_vlan_offload API.
> > > > >
> > > > > Regarding this API from ethdev.
> > > > >
> > > > > So this seems like a hack on ethdev. Currently there are 2 ways
> > > > > for user to
> > > > set Rx vlan offloads.
> > > > > One is through dev_configure which require the ports to be
> > > > > stopped. The
> > > > other is this API which can set even if the port is started.
> > > >
> > > > Yes there is an ability to enable/disable VLAN offloads without
> > > > stop/reconfigure the device.
> > > > Though I wouldn't call it 'a hack'.
> > > > From my perspective - it is a useful feature.
> > > > Same as it is possible in some cases to change MTU without
> > > > stopping device, etc.
>
> I think the function rte_eth_dev_configure(), which set up several things at a
> time, is a very bad idea from API perspective.
>
> In the VLAN example, we should have only one function to set this offload.
> And the PMD should advertise the capability of configuring VLAN on the fly.
> This function should return an error if called on the fly (started state) and
> PMD does not support it.
>
>
> > > > Consider scenario when PF has a corresponding VFs (PF is
> > > > controlled by
> > > > DPDK) Right now (at least with Intel HW) it is possible to:
> > > >
> > > > struct rte_eth_conf dev_conf;
> > > > dev_conf. rxmode.hw_vlan_filter = 1; ...
> > > > rte_eth_dev_configure(pf_port_id, 0, 0, &dev_conf);
> > > > rte_eth_dev_start(pf_port_id);
> > > >
> > > > In that scenario I don't have any RX/TX queues configured.
> > > > Though I still able to enable vlan filter, and it would work correctly for
> VFs.
> > > > Same for other per-port offloads.
> > >
> > > For the PF - enabling vlan filtering without any queues means
> > > nothing. The PF can receive no traffic, what different does it makes the
> vlan filtering is set?
> > > For the VF - I assume it will have queues, therefore for it vlan
> > > filtering has a meaning. However as I said above, the VF has the vlan filter
> because in intel this is per-device offload, so this is not a good example.
> >
> > Yes it is a per-device offload, and right now it is possible to
> > enable/disable it via dev_confgiure(); dev_start(); without
> > configuring/starting any RX/TX queues.
> > That's an ability I'd like to preserve.
> > So from my perspective it is a perfectly valid example.
>
> It is configuring VFs by setting the PF.
> Where is it documented?
> It looks to me as a device-specific side effect.
13/09/2017 13:16, Shahaf Shuler:
> Wednesday, September 13, 2017 12:28 PM, Thomas Monjalon:
> > I still think we must streamline ethdev API instead of complexifying.
> > We should drop the big "configure everything" and configure offloads one by
> > one, and per queue (the finer grain).
>
> The issue is, that there is some functionality which cannot be achieved when configuring offload per queue.
> For example - vlan filter on intel NICs. The PF can set it even without creating a single queue, in order to enable it for the VFs.
As it is a device-specific - not documented - side effect,
I won't consider it.
However I understand it may be better to be able to configure
per-port offloads with a dedicated per-port function.
I agree with the approach of the v3 of this series.
Let me give my overview of offloads:
We have simple offloads which are configured by just setting a flag.
The same flag can be set per-port or per-queue.
This offload can be set before starting or on the fly.
We currently have no generic way to set it on the fly.
We have also more complicate offloads which require more configuration.
They are set with the rte_flow API.
They can be per-port, per-queue, on the fly or not (AFAIK).
I think we must discuss "on the fly" capability.
It requires probably to set up simple offloads (flags) with a dedicated
function instead of using "configure" and "queue_setup" functions.
This new capability can be implemented in a different series.
Opinions?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, September 13, 2017 1:42 PM
> To: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> 13/09/2017 13:16, Shahaf Shuler:
> > Wednesday, September 13, 2017 12:28 PM, Thomas Monjalon:
> > > I still think we must streamline ethdev API instead of complexifying.
> > > We should drop the big "configure everything" and configure offloads one by
> > > one, and per queue (the finer grain).
> >
> > The issue is, that there is some functionality which cannot be achieved when configuring offload per queue.
> > For example - vlan filter on intel NICs. The PF can set it even without creating a single queue, in order to enable it for the VFs.
>
> As it is a device-specific - not documented - side effect,
> I won't consider it.
Hmm, are you saying that if there are gaps in our documentation it ok to break things?
Once again - you suggest to break existing functionality without providing any
alternative way to support it.
Surely I will NACK such proposal.
Konstantin
> However I understand it may be better to be able to configure
> per-port offloads with a dedicated per-port function.
> I agree with the approach of the v3 of this series.
>
> Let me give my overview of offloads:
>
> We have simple offloads which are configured by just setting a flag.
> The same flag can be set per-port or per-queue.
> This offload can be set before starting or on the fly.
> We currently have no generic way to set it on the fly.
>
> We have also more complicate offloads which require more configuration.
> They are set with the rte_flow API.
> They can be per-port, per-queue, on the fly or not (AFAIK).
>
> I think we must discuss "on the fly" capability.
> It requires probably to set up simple offloads (flags) with a dedicated
> function instead of using "configure" and "queue_setup" functions.
> This new capability can be implemented in a different series.
>
> Opinions?
Wednesday, September 13, 2017 3:42 PM, Thomas MonjalonL
> 13/09/2017 13:16, Shahaf Shuler:
> > Wednesday, September 13, 2017 12:28 PM, Thomas Monjalon:
> > > I still think we must streamline ethdev API instead of complexifying.
> > > We should drop the big "configure everything" and configure offloads
> > > one by one, and per queue (the finer grain).
> >
> > The issue is, that there is some functionality which cannot be achieved
> when configuring offload per queue.
> > For example - vlan filter on intel NICs. The PF can set it even without
> creating a single queue, in order to enable it for the VFs.
>
> As it is a device-specific - not documented - side effect, I won't consider it.
> However I understand it may be better to be able to configure per-port
> offloads with a dedicated per-port function.
> I agree with the approach of the v3 of this series.
>
> Let me give my overview of offloads:
>
> We have simple offloads which are configured by just setting a flag.
> The same flag can be set per-port or per-queue.
> This offload can be set before starting or on the fly.
> We currently have no generic way to set it on the fly.
>
> We have also more complicate offloads which require more configuration.
> They are set with the rte_flow API.
> They can be per-port, per-queue, on the fly or not (AFAIK).
>
> I think we must discuss "on the fly" capability.
> It requires probably to set up simple offloads (flags) with a dedicated
> function instead of using "configure" and "queue_setup" functions.
> This new capability can be implemented in a different series.
>
> Opinions?
Agree about the on the fly configuration for Tx/Rx offloads.
Currently the vlan case is an exception, we should have a generic API to set such offloads.
I assume that we also don't want to have dev_op function on PMD for each "on the flight" configuration like we have with the vlan offloads.
13/09/2017 14:56, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 13/09/2017 13:16, Shahaf Shuler:
> > > Wednesday, September 13, 2017 12:28 PM, Thomas Monjalon:
> > > > I still think we must streamline ethdev API instead of complexifying.
> > > > We should drop the big "configure everything" and configure offloads one by
> > > > one, and per queue (the finer grain).
> > >
> > > The issue is, that there is some functionality which cannot be achieved when configuring offload per queue.
> > > For example - vlan filter on intel NICs. The PF can set it even without creating a single queue, in order to enable it for the VFs.
> >
> > As it is a device-specific - not documented - side effect,
> > I won't consider it.
>
> Hmm, are you saying that if there are gaps in our documentation it ok to break things?
If it is not documented, we did not explicitly agree on it.
How an application knows that setting a PF settings will have
effect on related VFs?
> Once again - you suggest to break existing functionality without providing any
> alternative way to support it.
It is not a functionality, it is a side effect.
What happens if a VF changes this settings? error?
Is this error documented?
> Surely I will NACK such proposal.
Nothing to nack, I agree with v3 which doesn't break ixgbe VLAN settings.
Konstantin, I would like your opinion about the proposal below.
It is about making on the fly configuration more generic.
You say it is possible to configure VLAN on the fly,
and I think we should make it possible for other offload features.
> > However I understand it may be better to be able to configure
> > per-port offloads with a dedicated per-port function.
> > I agree with the approach of the v3 of this series.
> >
> > Let me give my overview of offloads:
> >
> > We have simple offloads which are configured by just setting a flag.
> > The same flag can be set per-port or per-queue.
> > This offload can be set before starting or on the fly.
> > We currently have no generic way to set it on the fly.
> >
> > We have also more complicate offloads which require more configuration.
> > They are set with the rte_flow API.
> > They can be per-port, per-queue, on the fly or not (AFAIK).
> >
> > I think we must discuss "on the fly" capability.
> > It requires probably to set up simple offloads (flags) with a dedicated
> > function instead of using "configure" and "queue_setup" functions.
> > This new capability can be implemented in a different series.
> >
> > Opinions?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, September 13, 2017 2:21 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> 13/09/2017 14:56, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 13/09/2017 13:16, Shahaf Shuler:
> > > > Wednesday, September 13, 2017 12:28 PM, Thomas Monjalon:
> > > > > I still think we must streamline ethdev API instead of complexifying.
> > > > > We should drop the big "configure everything" and configure offloads one by
> > > > > one, and per queue (the finer grain).
> > > >
> > > > The issue is, that there is some functionality which cannot be achieved when configuring offload per queue.
> > > > For example - vlan filter on intel NICs. The PF can set it even without creating a single queue, in order to enable it for the VFs.
> > >
> > > As it is a device-specific - not documented - side effect,
> > > I won't consider it.
> >
> > Hmm, are you saying that if there are gaps in our documentation it ok to break things?
>
> If it is not documented, we did not explicitly agree on it.
> How an application knows that setting a PF settings will have
> effect on related VFs?
>
> > Once again - you suggest to break existing functionality without providing any
> > alternative way to support it.
>
> It is not a functionality, it is a side effect.
I wouldn't agree with that.
DPDK does support PF for these devices.
It is responsibility of PF to provide to user ability to configure and control it's VF(s).
> What happens if a VF changes this settings? error?
Depends on particular offload and HW.
For ixgbe and igb for most cases VF is simply not physically capable to change
these things.
I think, that in some places error is returned, in other such request is silently ignored.
> Is this error documented?
>
> > Surely I will NACK such proposal.
>
> Nothing to nack, I agree with v3 which doesn't break ixgbe VLAN settings.
Ok then.
>
> Konstantin, I would like your opinion about the proposal below.
> It is about making on the fly configuration more generic.
> You say it is possible to configure VLAN on the fly,
> and I think we should make it possible for other offload features.
It would be a good thing, but I don't think it is possible for all offloads.
For some of them you still have to stop the queue(port) first.
Also I am not sure what exactly do you propose?
Is that something like that:
- wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
- Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
Introduce new functions:
int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
uint64_t rte_eth_get_port_rx_offload(portid);
uint64_t rte_eth_set_queue_rx_offload(portid, queueid);
And add new fileds:
rx_offload_port_dynamic_capa
rx_offload_queue_dynamic_capa
inside rte_eth_dev_info.
And it would be user responsibility to call set_port/queue_rx_offload()
somewhere before dev_start() for static offloads.
?
If so, then it seems reasonable to me.
Konstantin
>
> > > However I understand it may be better to be able to configure
> > > per-port offloads with a dedicated per-port function.
> > > I agree with the approach of the v3 of this series.
> > >
> > > Let me give my overview of offloads:
> > >
> > > We have simple offloads which are configured by just setting a flag.
> > > The same flag can be set per-port or per-queue.
> > > This offload can be set before starting or on the fly.
> > > We currently have no generic way to set it on the fly.
> > >
> > > We have also more complicate offloads which require more configuration.
> > > They are set with the rte_flow API.
> > > They can be per-port, per-queue, on the fly or not (AFAIK).
> > >
> > > I think we must discuss "on the fly" capability.
> > > It requires probably to set up simple offloads (flags) with a dedicated
> > > function instead of using "configure" and "queue_setup" functions.
> > > This new capability can be implemented in a different series.
> > >
> > > Opinions?
13/09/2017 23:42, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 13/09/2017 14:56, Ananyev, Konstantin:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Konstantin, I would like your opinion about the proposal below.
> > It is about making on the fly configuration more generic.
> > You say it is possible to configure VLAN on the fly,
> > and I think we should make it possible for other offload features.
>
> It would be a good thing, but I don't think it is possible for all offloads.
> For some of them you still have to stop the queue(port) first.
>
> Also I am not sure what exactly do you propose?
> Is that something like that:
> - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
> Introduce new functions:
>
> int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
>
> uint64_t rte_eth_get_port_rx_offload(portid);
> uint64_t rte_eth_set_queue_rx_offload(portid, queueid);
>
> And add new fileds:
> rx_offload_port_dynamic_capa
> rx_offload_queue_dynamic_capa
> inside rte_eth_dev_info.
>
> And it would be user responsibility to call set_port/queue_rx_offload()
> somewhere before dev_start() for static offloads.
> ?
Yes exactly.
> If so, then it seems reasonable to me.
Good, thank you
> > > > However I understand it may be better to be able to configure
> > > > per-port offloads with a dedicated per-port function.
> > > > I agree with the approach of the v3 of this series.
> > > >
> > > > Let me give my overview of offloads:
> > > >
> > > > We have simple offloads which are configured by just setting a flag.
> > > > The same flag can be set per-port or per-queue.
> > > > This offload can be set before starting or on the fly.
> > > > We currently have no generic way to set it on the fly.
> > > >
> > > > We have also more complicate offloads which require more configuration.
> > > > They are set with the rte_flow API.
> > > > They can be per-port, per-queue, on the fly or not (AFAIK).
> > > >
> > > > I think we must discuss "on the fly" capability.
> > > > It requires probably to set up simple offloads (flags) with a dedicated
> > > > function instead of using "configure" and "queue_setup" functions.
> > > > This new capability can be implemented in a different series.
> > > >
> > > > Opinions?
On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> 13/09/2017 23:42, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Konstantin, I would like your opinion about the proposal below.
> > > It is about making on the fly configuration more generic.
> > > You say it is possible to configure VLAN on the fly,
> > > and I think we should make it possible for other offload features.
> >
> > It would be a good thing, but I don't think it is possible for all offloads.
> > For some of them you still have to stop the queue(port) first.
> >
> > Also I am not sure what exactly do you propose?
> > Is that something like that:
> > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> > - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
> > Introduce new functions:
> >
> > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
Would be useful to have a valid mask here, to indicate what bits to use.
That way, you can adjust one bit without worrying about what other bits
you may change in the process. There are probably apps out there that
just want to toggle a single bit on, and off, at runtime while ignoring
others.
Alternatively, we can have set/unset functions which enable/disable
offloads, based on the mask.
> >
> > uint64_t rte_eth_get_port_rx_offload(portid);
> > uint64_t rte_eth_set_queue_rx_offload(portid, queueid);
s/set/get/
> >
> > And add new fileds:
> > rx_offload_port_dynamic_capa
> > rx_offload_queue_dynamic_capa
> > inside rte_eth_dev_info.
> >
> > And it would be user responsibility to call set_port/queue_rx_offload()
> > somewhere before dev_start() for static offloads.
> > ?
>
> Yes exactly.
>
> > If so, then it seems reasonable to me.
>
> Good, thank you
>
>
Sorry I'm a bit late to the review, but the above suggestion of separate
APIs for enabling offloads, seems much better than passing in the flags
in structures to the existing calls. From what I see all later revisions
of this patchset still use the existing flags parameter to setup calls
method.
Some advantages that I see of the separate APIs:
* allows some settings to be set before start, and others afterwards,
with an appropriate return value if dynamic config not supported.
* we can get fine grained error reporting from these - the set calls can
all return the mask indicating what offloads could not be applied -
zero means all ok, 1 means a problem with that setting. This may be
easier for the app to use than feature discovery in some cases.
* for those PMDs which support configuration at a per-queue level, it
can allow the user to specify the per-port settings as a default, and
then override that value at the queue level, if you just want one queue
different from the rest.
Regards,
/Bruce
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, September 18, 2017 11:32 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > 13/09/2017 23:42, Ananyev, Konstantin:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Konstantin, I would like your opinion about the proposal below.
> > > > It is about making on the fly configuration more generic.
> > > > You say it is possible to configure VLAN on the fly,
> > > > and I think we should make it possible for other offload features.
> > >
> > > It would be a good thing, but I don't think it is possible for all offloads.
> > > For some of them you still have to stop the queue(port) first.
> > >
> > > Also I am not sure what exactly do you propose?
> > > Is that something like that:
> > > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> > > - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
> > > Introduce new functions:
> > >
> > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
> Would be useful to have a valid mask here, to indicate what bits to use.
> That way, you can adjust one bit without worrying about what other bits
> you may change in the process. There are probably apps out there that
> just want to toggle a single bit on, and off, at runtime while ignoring
> others.
> Alternatively, we can have set/unset functions which enable/disable
> offloads, based on the mask.
My thought was that people would do:
uint64_t offload = rte_eth_get_port_rx_offload(port);
offload |= RX_OFFLOAD_X;
offload &= ~RX_OFFLOAD_Y;
rte_eth_set_port_rx_offload(port, offload);
In that case, I think we don't really need a mask.
>
> > >
> > > uint64_t rte_eth_get_port_rx_offload(portid);
> > > uint64_t rte_eth_set_queue_rx_offload(portid, queueid);
> s/set/get/
> > >
> > > And add new fileds:
> > > rx_offload_port_dynamic_capa
> > > rx_offload_queue_dynamic_capa
> > > inside rte_eth_dev_info.
> > >
> > > And it would be user responsibility to call set_port/queue_rx_offload()
> > > somewhere before dev_start() for static offloads.
> > > ?
> >
> > Yes exactly.
> >
> > > If so, then it seems reasonable to me.
> >
> > Good, thank you
> >
> >
> Sorry I'm a bit late to the review, but the above suggestion of separate
> APIs for enabling offloads, seems much better than passing in the flags
> in structures to the existing calls. From what I see all later revisions
> of this patchset still use the existing flags parameter to setup calls
> method.
>
> Some advantages that I see of the separate APIs:
> * allows some settings to be set before start, and others afterwards,
> with an appropriate return value if dynamic config not supported.
> * we can get fine grained error reporting from these - the set calls can
> all return the mask indicating what offloads could not be applied -
> zero means all ok, 1 means a problem with that setting. This may be
> easier for the app to use than feature discovery in some cases.
> * for those PMDs which support configuration at a per-queue level, it
> can allow the user to specify the per-port settings as a default, and
> then override that value at the queue level, if you just want one queue
> different from the rest.
I think we all in favor to have a separate API here.
Though from the discussion we had at latest TB, I am not sure it is doable
in 17.11 timeframe.
Konstantin
On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote:
>
>
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Monday, September 18, 2017 11:32 AM
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler
> > <shahafs@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
> >
> > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > > 13/09/2017 23:42, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > Konstantin, I would like your opinion about the proposal below.
> > > > > It is about making on the fly configuration more generic.
> > > > > You say it is possible to configure VLAN on the fly,
> > > > > and I think we should make it possible for other offload features.
> > > >
> > > > It would be a good thing, but I don't think it is possible for all offloads.
> > > > For some of them you still have to stop the queue(port) first.
> > > >
> > > > Also I am not sure what exactly do you propose?
> > > > Is that something like that:
> > > > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> > > > - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
> > > > Introduce new functions:
> > > >
> > > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
> > Would be useful to have a valid mask here, to indicate what bits to use.
> > That way, you can adjust one bit without worrying about what other bits
> > you may change in the process. There are probably apps out there that
> > just want to toggle a single bit on, and off, at runtime while ignoring
> > others.
> > Alternatively, we can have set/unset functions which enable/disable
> > offloads, based on the mask.
>
> My thought was that people would do:
>
> uint64_t offload = rte_eth_get_port_rx_offload(port);
> offload |= RX_OFFLOAD_X;
> offload &= ~RX_OFFLOAD_Y;
> rte_eth_set_port_rx_offload(port, offload);
>
> In that case, I think we don't really need a mask.
>
Sure, that can work, I'm not concerned either way.
Overall, I think my slight preference would be to have set/unset,
enable/disable functions to make it clear what is happening, rather than
having to worry about the complete set each time.
uint64_t rte_eth_port_rx_offload_enable(port_id, offload_mask)
uint64_t rte_eth_port_rx_offload_disable(port_id, offload_mask)
each returning the bits failing (or bits changed if you like, but I prefer
bits failing as return value, since it means 0 == no_error).
/Bruce
On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote:
>
>
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Monday, September 18, 2017 11:32 AM
> > To: Thomas Monjalon <thomas@monjalon.net>
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler
> > <shahafs@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
> >
> > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > > 13/09/2017 23:42, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > Konstantin, I would like your opinion about the proposal below.
> > > > > It is about making on the fly configuration more generic.
> > > > > You say it is possible to configure VLAN on the fly,
> > > > > and I think we should make it possible for other offload features.
> > > >
> > > > It would be a good thing, but I don't think it is possible for all offloads.
> > > > For some of them you still have to stop the queue(port) first.
> > > >
> > > > Also I am not sure what exactly do you propose?
> > > > Is that something like that:
> > > > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> > > > - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
> > > > Introduce new functions:
> > > >
> > > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
> > Would be useful to have a valid mask here, to indicate what bits to use.
> > That way, you can adjust one bit without worrying about what other bits
> > you may change in the process. There are probably apps out there that
> > just want to toggle a single bit on, and off, at runtime while ignoring
> > others.
> > Alternatively, we can have set/unset functions which enable/disable
> > offloads, based on the mask.
>
> My thought was that people would do:
>
> uint64_t offload = rte_eth_get_port_rx_offload(port);
> offload |= RX_OFFLOAD_X;
> offload &= ~RX_OFFLOAD_Y;
> rte_eth_set_port_rx_offload(port, offload);
>
> In that case, I think we don't really need a mask.
>
> >
> > > >
> > > > uint64_t rte_eth_get_port_rx_offload(portid);
> > > > uint64_t rte_eth_set_queue_rx_offload(portid, queueid);
> > s/set/get/
> > > >
> > > > And add new fileds:
> > > > rx_offload_port_dynamic_capa
> > > > rx_offload_queue_dynamic_capa
> > > > inside rte_eth_dev_info.
> > > >
> > > > And it would be user responsibility to call set_port/queue_rx_offload()
> > > > somewhere before dev_start() for static offloads.
> > > > ?
> > >
> > > Yes exactly.
> > >
> > > > If so, then it seems reasonable to me.
> > >
> > > Good, thank you
> > >
> > >
> > Sorry I'm a bit late to the review, but the above suggestion of separate
> > APIs for enabling offloads, seems much better than passing in the flags
> > in structures to the existing calls. From what I see all later revisions
> > of this patchset still use the existing flags parameter to setup calls
> > method.
> >
> > Some advantages that I see of the separate APIs:
> > * allows some settings to be set before start, and others afterwards,
> > with an appropriate return value if dynamic config not supported.
> > * we can get fine grained error reporting from these - the set calls can
> > all return the mask indicating what offloads could not be applied -
> > zero means all ok, 1 means a problem with that setting. This may be
> > easier for the app to use than feature discovery in some cases.
> > * for those PMDs which support configuration at a per-queue level, it
> > can allow the user to specify the per-port settings as a default, and
> > then override that value at the queue level, if you just want one queue
> > different from the rest.
>
> I think we all in favor to have a separate API here.
> Though from the discussion we had at latest TB, I am not sure it is doable
> in 17.11 timeframe.
Ok, so does that imply no change in this release, and that the existing
set is to be ignored?
/Bruce
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, September 18, 2017 12:05 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
>
> On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Monday, September 18, 2017 11:32 AM
> > > To: Thomas Monjalon <thomas@monjalon.net>
> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler
> > > <shahafs@mellanox.com>
> > > Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
> > >
> > > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > > > 13/09/2017 23:42, Ananyev, Konstantin:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > Konstantin, I would like your opinion about the proposal below.
> > > > > > It is about making on the fly configuration more generic.
> > > > > > You say it is possible to configure VLAN on the fly,
> > > > > > and I think we should make it possible for other offload features.
> > > > >
> > > > > It would be a good thing, but I don't think it is possible for all offloads.
> > > > > For some of them you still have to stop the queue(port) first.
> > > > >
> > > > > Also I am not sure what exactly do you propose?
> > > > > Is that something like that:
> > > > > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> > > > > - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
> > > > > Introduce new functions:
> > > > >
> > > > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > > > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
> > > Would be useful to have a valid mask here, to indicate what bits to use.
> > > That way, you can adjust one bit without worrying about what other bits
> > > you may change in the process. There are probably apps out there that
> > > just want to toggle a single bit on, and off, at runtime while ignoring
> > > others.
> > > Alternatively, we can have set/unset functions which enable/disable
> > > offloads, based on the mask.
> >
> > My thought was that people would do:
> >
> > uint64_t offload = rte_eth_get_port_rx_offload(port);
> > offload |= RX_OFFLOAD_X;
> > offload &= ~RX_OFFLOAD_Y;
> > rte_eth_set_port_rx_offload(port, offload);
> >
> > In that case, I think we don't really need a mask.
> >
> > >
> > > > >
> > > > > uint64_t rte_eth_get_port_rx_offload(portid);
> > > > > uint64_t rte_eth_set_queue_rx_offload(portid, queueid);
> > > s/set/get/
> > > > >
> > > > > And add new fileds:
> > > > > rx_offload_port_dynamic_capa
> > > > > rx_offload_queue_dynamic_capa
> > > > > inside rte_eth_dev_info.
> > > > >
> > > > > And it would be user responsibility to call set_port/queue_rx_offload()
> > > > > somewhere before dev_start() for static offloads.
> > > > > ?
> > > >
> > > > Yes exactly.
> > > >
> > > > > If so, then it seems reasonable to me.
> > > >
> > > > Good, thank you
> > > >
> > > >
> > > Sorry I'm a bit late to the review, but the above suggestion of separate
> > > APIs for enabling offloads, seems much better than passing in the flags
> > > in structures to the existing calls. From what I see all later revisions
> > > of this patchset still use the existing flags parameter to setup calls
> > > method.
> > >
> > > Some advantages that I see of the separate APIs:
> > > * allows some settings to be set before start, and others afterwards,
> > > with an appropriate return value if dynamic config not supported.
> > > * we can get fine grained error reporting from these - the set calls can
> > > all return the mask indicating what offloads could not be applied -
> > > zero means all ok, 1 means a problem with that setting. This may be
> > > easier for the app to use than feature discovery in some cases.
> > > * for those PMDs which support configuration at a per-queue level, it
> > > can allow the user to specify the per-port settings as a default, and
> > > then override that value at the queue level, if you just want one queue
> > > different from the rest.
> >
> > I think we all in favor to have a separate API here.
> > Though from the discussion we had at latest TB, I am not sure it is doable
> > in 17.11 timeframe.
>
> Ok, so does that imply no change in this release, and that the existing
> set is to be ignored?
No, my understanding the current plan is to go forward with Shahaf patches,
and then apply another one (new set/get API) on top of them.
Konstantin
18/09/2017 13:04, Bruce Richardson:
> On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote:
> > From: Richardson, Bruce
> > > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > > > 13/09/2017 23:42, Ananyev, Konstantin:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > Konstantin, I would like your opinion about the proposal below.
> > > > > > It is about making on the fly configuration more generic.
> > > > > > You say it is possible to configure VLAN on the fly,
> > > > > > and I think we should make it possible for other offload features.
> > > > >
> > > > > It would be a good thing, but I don't think it is possible for all offloads.
> > > > > For some of them you still have to stop the queue(port) first.
> > > > >
> > > > > Also I am not sure what exactly do you propose?
> > > > > Is that something like that:
> > > > > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> > > > > - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf
> > > > > Introduce new functions:
> > > > >
> > > > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > > > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
> > > Would be useful to have a valid mask here, to indicate what bits to use.
> > > That way, you can adjust one bit without worrying about what other bits
> > > you may change in the process. There are probably apps out there that
> > > just want to toggle a single bit on, and off, at runtime while ignoring
> > > others.
> > > Alternatively, we can have set/unset functions which enable/disable
> > > offloads, based on the mask.
> >
> > My thought was that people would do:
> >
> > uint64_t offload = rte_eth_get_port_rx_offload(port);
> > offload |= RX_OFFLOAD_X;
> > offload &= ~RX_OFFLOAD_Y;
> > rte_eth_set_port_rx_offload(port, offload);
> >
> > In that case, I think we don't really need a mask.
> >
> Sure, that can work, I'm not concerned either way.
>
> Overall, I think my slight preference would be to have set/unset,
> enable/disable functions to make it clear what is happening, rather than
> having to worry about the complete set each time.
>
> uint64_t rte_eth_port_rx_offload_enable(port_id, offload_mask)
> uint64_t rte_eth_port_rx_offload_disable(port_id, offload_mask)
>
> each returning the bits failing (or bits changed if you like, but I prefer
> bits failing as return value, since it means 0 == no_error).
I think we need both: "get" functions + "mask" parameters in "set" functions.
18/09/2017 13:11, Ananyev, Konstantin:
> From: Richardson, Bruce
> > On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote:
> > > From: Richardson, Bruce
> > > > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > > > > 13/09/2017 23:42, Ananyev, Konstantin:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > Konstantin, I would like your opinion about the proposal below.
> > > > > > > It is about making on the fly configuration more generic.
> > > > > > > You say it is possible to configure VLAN on the fly,
> > > > > > > and I think we should make it possible for other offload features.
> > > > > >
> > > > > > It would be a good thing, but I don't think it is possible for all offloads.
> > > > > > For some of them you still have to stop the queue(port) first.
[...]
[technical details skipped]
[...]
> > > > > > If so, then it seems reasonable to me.
> > > > >
> > > > > Good, thank you
> > > > >
> > > > >
> > > > Sorry I'm a bit late to the review, but the above suggestion of separate
> > > > APIs for enabling offloads, seems much better than passing in the flags
> > > > in structures to the existing calls. From what I see all later revisions
> > > > of this patchset still use the existing flags parameter to setup calls
> > > > method.
> > > >
> > > > Some advantages that I see of the separate APIs:
> > > > * allows some settings to be set before start, and others afterwards,
> > > > with an appropriate return value if dynamic config not supported.
> > > > * we can get fine grained error reporting from these - the set calls can
> > > > all return the mask indicating what offloads could not be applied -
> > > > zero means all ok, 1 means a problem with that setting. This may be
> > > > easier for the app to use than feature discovery in some cases.
> > > > * for those PMDs which support configuration at a per-queue level, it
> > > > can allow the user to specify the per-port settings as a default, and
> > > > then override that value at the queue level, if you just want one queue
> > > > different from the rest.
> > >
> > > I think we all in favor to have a separate API here.
> > > Though from the discussion we had at latest TB, I am not sure it is doable
> > > in 17.11 timeframe.
> >
> > Ok, so does that imply no change in this release, and that the existing
> > set is to be ignored?
>
> No, my understanding the current plan is to go forward with Shahaf patches,
> and then apply another one (new set/get API) on top of them.
Yes, it is what we agreed (hope to see it in minutes).
If someone can do these new patches in 17.11 timeframe, it's great!
Bruce, do you want to make it a try?
On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> 18/09/2017 13:11, Ananyev, Konstantin:
> > From: Richardson, Bruce
> > > On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote:
> > > > From: Richardson, Bruce
> > > > > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > > > > > 13/09/2017 23:42, Ananyev, Konstantin:
> > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > > Konstantin, I would like your opinion about the proposal below.
> > > > > > > > It is about making on the fly configuration more generic.
> > > > > > > > You say it is possible to configure VLAN on the fly,
> > > > > > > > and I think we should make it possible for other offload features.
> > > > > > >
> > > > > > > It would be a good thing, but I don't think it is possible for all offloads.
> > > > > > > For some of them you still have to stop the queue(port) first.
> [...]
> [technical details skipped]
> [...]
> > > > > > > If so, then it seems reasonable to me.
> > > > > >
> > > > > > Good, thank you
> > > > > >
> > > > > >
> > > > > Sorry I'm a bit late to the review, but the above suggestion of separate
> > > > > APIs for enabling offloads, seems much better than passing in the flags
> > > > > in structures to the existing calls. From what I see all later revisions
> > > > > of this patchset still use the existing flags parameter to setup calls
> > > > > method.
> > > > >
> > > > > Some advantages that I see of the separate APIs:
> > > > > * allows some settings to be set before start, and others afterwards,
> > > > > with an appropriate return value if dynamic config not supported.
> > > > > * we can get fine grained error reporting from these - the set calls can
> > > > > all return the mask indicating what offloads could not be applied -
> > > > > zero means all ok, 1 means a problem with that setting. This may be
> > > > > easier for the app to use than feature discovery in some cases.
> > > > > * for those PMDs which support configuration at a per-queue level, it
> > > > > can allow the user to specify the per-port settings as a default, and
> > > > > then override that value at the queue level, if you just want one queue
> > > > > different from the rest.
> > > >
> > > > I think we all in favor to have a separate API here.
> > > > Though from the discussion we had at latest TB, I am not sure it is doable
> > > > in 17.11 timeframe.
> > >
> > > Ok, so does that imply no change in this release, and that the existing
> > > set is to be ignored?
> >
> > No, my understanding the current plan is to go forward with Shahaf patches,
> > and then apply another one (new set/get API) on top of them.
>
> Yes, it is what we agreed (hope to see it in minutes).
> If someone can do these new patches in 17.11 timeframe, it's great!
> Bruce, do you want to make it a try?
If I have the chance, I can try, but given how short time is and that
userspace is on next week, I very much doubt I'll even get it started.
/Bruce
Monday, September 18, 2017 2:38 PM, Bruce Richardson
> On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> > 18/09/2017 13:11, Ananyev, Konstantin:
> > > From: Richardson, Bruce
> > > > >
> > > > > I think we all in favor to have a separate API here.
> > > > > Though from the discussion we had at latest TB, I am not sure it
> > > > > is doable in 17.11 timeframe.
> > > >
> > > > Ok, so does that imply no change in this release, and that the
> > > > existing set is to be ignored?
> > >
> > > No, my understanding the current plan is to go forward with Shahaf
> > > patches, and then apply another one (new set/get API) on top of them.
> >
> > Yes, it is what we agreed (hope to see it in minutes).
> > If someone can do these new patches in 17.11 timeframe, it's great!
> > Bruce, do you want to make it a try?
>
> If I have the chance, I can try, but given how short time is and that userspace
> is on next week, I very much doubt I'll even get it started.
I wasn't aware to the techboard decision on the extra patchset needed.
I think it will be wrong to introduce an API on 17.11 and change it again on 18.02.
I will do my best to make everything ready for 17.11 so we can have one solid API on top of which all PMDs and application will be converted. Considering some Holidays and the DPDK summit I won't have much time to work on it.
The plan is as follows:
1. complete the last comment on the current series and integrate it.
2. send a new patchset to convert to the API suggested above.
Aggregating the different suggestions I come up with the below. if this is agreed, then I will move with the implementation.
(I thought it is good to return error values for the get function).
**
* Get Tx offloads set on a specific port.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param offloads
* A pointer to uint64_t where the offloads flags
* will be filled using DEV_TX_OFFLOAD_* flags.
* @return
* - (0) if successful.
* - (-ENOTSUP or -ENODEV) on failure.
*/
int rte_eth_get_port_tx_offloads(uint8_t port_id, uint64_t *offloads);
**
* Get Tx offloads set on a specific queue.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param queue_id
* The queue identifier.
* @param offloads
* A pointer to uint64_t where the offloads flags
* will be filled using DEV_TX_OFFLOAD_* flags.
* @return
* - (0) if successful.
* - (-ENOTSUP or -ENODEV) on failure.
*/
int rte_eth_get_queue_tx_offloads(uint8_t port_id, uint16_t queue_id,
uint64_t *offloads);
**
* Set Tx offloads on a specific port.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param offloads_mask
* Indicates which offloads to be set using DEV_TX_OFFLOAD_* flags.
* @return
* (0) if all offloads set successfully, otherwise offloads
* flags which were not set.
*
*/
uint64_t rte_eth_set_port_tx_offloads(uint8_t port_id, uint64_t offloads_mask);
/**
* Set Tx offloads on a specific queue.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param queue_id
* The queue identifier.
* @param offloads_mask
* Indicates which offloads to be set using DEV_TX_OFFLOAD_* flags.
* @return
* (0) if all offloads set successfully, otherwise offloads
* flags which were not set.
*
*/
uint64_t rte_eth_set_queue_tx_offloads(uint8_t port_id, uint16_t queue_id,
uint64_t offloads_mask);
/**
* Get Rx offloads set on a specific port.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param offloads
* A pointer to uint64_t where the offloads flags
* will be filled using DEV_RX_OFFLOAD_* flags.
* @return
* - (0) if successful.
* - (-ENOTSUP or -ENODEV) on failure.
*/
int rte_eth_get_port_rx_offloads(uint8_t port_id, uint64_t *offloads);
/**
* Get Rx offloads set on a specific queue.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param queue_id
* The queue identifier.
* @param offloads
* A pointer to uint64_t where the offloads flags
* will be filled using DEV_RX_OFFLOAD_* flags.
* @return
* - (0) if successful.
* - (-ENOTSUP or -ENODEV) on failure.
*/
int rte_eth_get_queue_rx_offlaods(uint8_t port_id, uint16_t queue_id,
uint64_t *offloads);
/**
* Set Rx offloads on a specific port.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param offloads_mask
* Indicates which offloads to be set using DEV_RX_OFFLOAD_* flags.
* @return
* (0) if all offloads set successfully, otherwise offloads
* flags which were not set.
*
*/
uint64_t rte_eth_set_port_rx_offloads(uint8_t port_id, uint64_t offloads_mask);
/**
* Set Rx offloads on a specific port.
*
* @param port_id
* The port identifier of the Ethernet device.
* @param queue_id
* The queue identifier.
* @param offloads_mask
* Indicates which offloads to be set using DEV_RX_OFFLOAD_* flags.
* @return
* (0) if all offloads set successfully, otherwise offloads
* flags which were not set.
*
*/
uint64_t rte_eth_set_queue_rx_offloads(uint8_t port_id, uint16_t queue_id,
uint64_t offloads_mask);
>
> /Bruce
18/09/2017 16:27, Shahaf Shuler:
> Monday, September 18, 2017 2:38 PM, Bruce Richardson
> > On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> > > 18/09/2017 13:11, Ananyev, Konstantin:
> > > > From: Richardson, Bruce
> > > > > >
> > > > > > I think we all in favor to have a separate API here.
> > > > > > Though from the discussion we had at latest TB, I am not sure it
> > > > > > is doable in 17.11 timeframe.
> > > > >
> > > > > Ok, so does that imply no change in this release, and that the
> > > > > existing set is to be ignored?
> > > >
> > > > No, my understanding the current plan is to go forward with Shahaf
> > > > patches, and then apply another one (new set/get API) on top of them.
> > >
> > > Yes, it is what we agreed (hope to see it in minutes).
> > > If someone can do these new patches in 17.11 timeframe, it's great!
> > > Bruce, do you want to make it a try?
> >
> > If I have the chance, I can try, but given how short time is and that userspace
> > is on next week, I very much doubt I'll even get it started.
>
> I wasn't aware to the techboard decision on the extra patchset needed.
> I think it will be wrong to introduce an API on 17.11 and change it again on 18.02.
> I will do my best to make everything ready for 17.11 so we can have one solid API on top of which all PMDs and application will be converted. Considering some Holidays and the DPDK summit I won't have much time to work on it.
>
> The plan is as follows:
> 1. complete the last comment on the current series and integrate it.
> 2. send a new patchset to convert to the API suggested above.
Thank you Shahaf.
> Aggregating the different suggestions I come up with the below. if this is agreed, then I will move with the implementation.
> (I thought it is good to return error values for the get function).
[...]
> **
> * Set Tx offloads on a specific port.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param offloads_mask
> * Indicates which offloads to be set using DEV_TX_OFFLOAD_* flags.
> * @return
> * (0) if all offloads set successfully, otherwise offloads
> * flags which were not set.
> *
> */
> uint64_t rte_eth_set_port_tx_offloads(uint8_t port_id, uint64_t offloads_mask);
You need to have a parameter for the offloads value,
different of offloads mask:
set(port, value, mask)
Or as proposed by Bruce, you need 2 functions:
enable(port, mask)
disable(port, mask)
On Mon, Sep 18, 2017 at 02:27:25PM +0000, Shahaf Shuler wrote:
> Monday, September 18, 2017 2:38 PM, Bruce Richardson
> > On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> > > 18/09/2017 13:11, Ananyev, Konstantin:
> > > > From: Richardson, Bruce
> > > > > >
> > > > > > I think we all in favor to have a separate API here.
> > > > > > Though from the discussion we had at latest TB, I am not sure it
> > > > > > is doable in 17.11 timeframe.
> > > > >
> > > > > Ok, so does that imply no change in this release, and that the
> > > > > existing set is to be ignored?
> > > >
> > > > No, my understanding the current plan is to go forward with Shahaf
> > > > patches, and then apply another one (new set/get API) on top of them.
> > >
> > > Yes, it is what we agreed (hope to see it in minutes).
> > > If someone can do these new patches in 17.11 timeframe, it's great!
> > > Bruce, do you want to make it a try?
> >
> > If I have the chance, I can try, but given how short time is and that userspace
> > is on next week, I very much doubt I'll even get it started.
>
> I wasn't aware to the techboard decision on the extra patchset needed.
> I think it will be wrong to introduce an API on 17.11 and change it again on 18.02.
> I will do my best to make everything ready for 17.11 so we can have one solid API on top of which all PMDs and application will be converted. Considering some Holidays and the DPDK summit I won't have much time to work on it.
>
> The plan is as follows:
> 1. complete the last comment on the current series and integrate it.
> 2. send a new patchset to convert to the API suggested above.
>
> Aggregating the different suggestions I come up with the below. if this is agreed, then I will move with the implementation.
> (I thought it is good to return error values for the get function).
I'd rather you didn't. :-) The only realistic error I would consider is
an invalid port id, and I think returning 0 - no offloads - is fine in
those cases. The user will pretty quickly discover it's an invalid port
id anyway, so I prefer a get function to just return the value as a
return value and be done with it!
Otherwise, these will do fine. I would prefer some way to only change
one offload at a time without having to call "get" and do bit twiddling
before a call to "set", but this will be ok, if others are happy with
it.
If we at least get the return value as the mask of enabled offloads we
can at least shorten some cases as e.g.
rte_eth_set_port_tx_offloads(port_id, rte_eth_get_port_tx_offloads(port_id) | OFFLOAD_X);
/Bruce
>
> **
> * Get Tx offloads set on a specific port.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param offloads
> * A pointer to uint64_t where the offloads flags
> * will be filled using DEV_TX_OFFLOAD_* flags.
> * @return
> * - (0) if successful.
> * - (-ENOTSUP or -ENODEV) on failure.
> */
> int rte_eth_get_port_tx_offloads(uint8_t port_id, uint64_t *offloads);
>
> **
> * Get Tx offloads set on a specific queue.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param queue_id
> * The queue identifier.
> * @param offloads
> * A pointer to uint64_t where the offloads flags
> * will be filled using DEV_TX_OFFLOAD_* flags.
> * @return
> * - (0) if successful.
> * - (-ENOTSUP or -ENODEV) on failure.
> */
> int rte_eth_get_queue_tx_offloads(uint8_t port_id, uint16_t queue_id,
> uint64_t *offloads);
> **
> * Set Tx offloads on a specific port.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param offloads_mask
> * Indicates which offloads to be set using DEV_TX_OFFLOAD_* flags.
> * @return
> * (0) if all offloads set successfully, otherwise offloads
> * flags which were not set.
> *
> */
> uint64_t rte_eth_set_port_tx_offloads(uint8_t port_id, uint64_t offloads_mask);
>
> /**
> * Set Tx offloads on a specific queue.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param queue_id
> * The queue identifier.
> * @param offloads_mask
> * Indicates which offloads to be set using DEV_TX_OFFLOAD_* flags.
> * @return
> * (0) if all offloads set successfully, otherwise offloads
> * flags which were not set.
> *
> */
> uint64_t rte_eth_set_queue_tx_offloads(uint8_t port_id, uint16_t queue_id,
> uint64_t offloads_mask);
> /**
> * Get Rx offloads set on a specific port.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param offloads
> * A pointer to uint64_t where the offloads flags
> * will be filled using DEV_RX_OFFLOAD_* flags.
> * @return
> * - (0) if successful.
> * - (-ENOTSUP or -ENODEV) on failure.
> */
> int rte_eth_get_port_rx_offloads(uint8_t port_id, uint64_t *offloads);
>
> /**
> * Get Rx offloads set on a specific queue.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param queue_id
> * The queue identifier.
> * @param offloads
> * A pointer to uint64_t where the offloads flags
> * will be filled using DEV_RX_OFFLOAD_* flags.
> * @return
> * - (0) if successful.
> * - (-ENOTSUP or -ENODEV) on failure.
> */
> int rte_eth_get_queue_rx_offlaods(uint8_t port_id, uint16_t queue_id,
> uint64_t *offloads);
>
> /**
> * Set Rx offloads on a specific port.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param offloads_mask
> * Indicates which offloads to be set using DEV_RX_OFFLOAD_* flags.
> * @return
> * (0) if all offloads set successfully, otherwise offloads
> * flags which were not set.
> *
> */
> uint64_t rte_eth_set_port_rx_offloads(uint8_t port_id, uint64_t offloads_mask);
>
> /**
> * Set Rx offloads on a specific port.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @param queue_id
> * The queue identifier.
> * @param offloads_mask
> * Indicates which offloads to be set using DEV_RX_OFFLOAD_* flags.
> * @return
> * (0) if all offloads set successfully, otherwise offloads
> * flags which were not set.
> *
> */
> uint64_t rte_eth_set_queue_rx_offloads(uint8_t port_id, uint16_t queue_id,
> uint64_t offloads_mask);
>
> >
> > /Bruce
Monday, September 18, 2017 5:45 PM, Bruce Richardson:
>
> On Mon, Sep 18, 2017 at 02:27:25PM +0000, Shahaf Shuler wrote:
> > Monday, September 18, 2017 2:38 PM, Bruce Richardson
> > > On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> > > > 18/09/2017 13:11, Ananyev, Konstantin:
> > > > > From: Richardson, Bruce
> > > > > > >
> > > > > > > I think we all in favor to have a separate API here.
> > > > > > > Though from the discussion we had at latest TB, I am not
> > > > > > > sure it is doable in 17.11 timeframe.
> > > > > >
> > > > > > Ok, so does that imply no change in this release, and that the
> > > > > > existing set is to be ignored?
> > > > >
> > > > > No, my understanding the current plan is to go forward with
> > > > > Shahaf patches, and then apply another one (new set/get API) on top
> of them.
> > > >
> > > > Yes, it is what we agreed (hope to see it in minutes).
> > > > If someone can do these new patches in 17.11 timeframe, it's great!
> > > > Bruce, do you want to make it a try?
> > >
> > > If I have the chance, I can try, but given how short time is and
> > > that userspace is on next week, I very much doubt I'll even get it started.
> >
> > I wasn't aware to the techboard decision on the extra patchset needed.
> > I think it will be wrong to introduce an API on 17.11 and change it again on
> 18.02.
> > I will do my best to make everything ready for 17.11 so we can have one
> solid API on top of which all PMDs and application will be converted.
> Considering some Holidays and the DPDK summit I won't have much time to
> work on it.
> >
> > The plan is as follows:
> > 1. complete the last comment on the current series and integrate it.
> > 2. send a new patchset to convert to the API suggested above.
> >
> > Aggregating the different suggestions I come up with the below. if this is
> agreed, then I will move with the implementation.
> > (I thought it is good to return error values for the get function).
>
> I'd rather you didn't. :-) The only realistic error I would consider is an invalid
> port id, and I think returning 0 - no offloads - is fine in those cases. The user
> will pretty quickly discover it's an invalid port id anyway, so I prefer a get
> function to just return the value as a return value and be done with it!
It would be simpler, however am not sure invalid port is the only error to consider. Another possible error can be the PMD is not supporting this function.
On that case returning 0 is not good enough. The application cannot know why the offload is not set, is it because it is set wrong or the PMD just don't support this API (which leads me to my next point).
Declare:
API1 = The API being worked so far.
AP2 = The suggested API being discussed here.
API1 was designed for easy adoption from both PMDs and application. Application can use either old/new API on top of PMD which support one of them thanks to the convert function. There was no hard demand to force all of the PMDs to support it at once.
With API2 this model breaks. Application which moved to the new offloads API cannot work with PMD which supports the old one.
If I aggregate the pros for API2:
From Bruce:
>* allows some settings to be set before start, and others afterwards,
> with an appropriate return value if dynamic config not supported.
>* we can get fine grained error reporting from these - the set calls can
> all return the mask indicating what offloads could not be applied -
> zero means all ok, 1 means a problem with that setting. This may be
> easier for the app to use than feature discovery in some cases.
For that functionality I think the get function are enough (application set offload and then check which of them is actually set).
The set function are more for the on the fly configuration.
>* for those PMDs which support configuration at a per-queue level, it
> can allow the user to specify the per-port settings as a default, and
> then override that value at the queue level, if you just want one queue
> different from the rest.
This can be done with API1 as well.
From Thomas:
> make the on the flight vlan offloads configuration more generic.
The cons:
1. hard requirement from the PMDs to support the new API.
I can commit for mlx5 and mlx4 PMDs. I know other PMD maintainers plan to convert their PMDs as well. If we take this approach we must make sure they all move.
There is another possible API (API3):
1. keep the per-port, per-queue configuration.
2. add the get function for better error reporting and visibility for application.
3. keep the current on the flight vlan offloading configuration as an exception. In case there will be a need to configure more offloads on the flight we can move to API2.
With API1 I am obviously OK.
I agree API2 is more generic.
API3 is a nice compromise, if we don't want to force all PMD to convert.
--Shahaf
18/09/2017 20:18, Shahaf Shuler:
> Monday, September 18, 2017 5:45 PM, Bruce Richardson:
> >
> > On Mon, Sep 18, 2017 at 02:27:25PM +0000, Shahaf Shuler wrote:
> > > Monday, September 18, 2017 2:38 PM, Bruce Richardson
> > > > On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> > > > > 18/09/2017 13:11, Ananyev, Konstantin:
> > > > > > From: Richardson, Bruce
> > > > > > > >
> > > > > > > > I think we all in favor to have a separate API here.
> > > > > > > > Though from the discussion we had at latest TB, I am not
> > > > > > > > sure it is doable in 17.11 timeframe.
> > > > > > >
> > > > > > > Ok, so does that imply no change in this release, and that the
> > > > > > > existing set is to be ignored?
> > > > > >
> > > > > > No, my understanding the current plan is to go forward with
> > > > > > Shahaf patches, and then apply another one (new set/get API) on top
> > of them.
> > > > >
> > > > > Yes, it is what we agreed (hope to see it in minutes).
> > > > > If someone can do these new patches in 17.11 timeframe, it's great!
> > > > > Bruce, do you want to make it a try?
> > > >
> > > > If I have the chance, I can try, but given how short time is and
> > > > that userspace is on next week, I very much doubt I'll even get it started.
> > >
> > > I wasn't aware to the techboard decision on the extra patchset needed.
> > > I think it will be wrong to introduce an API on 17.11 and change it again on
> > 18.02.
> > > I will do my best to make everything ready for 17.11 so we can have one
> > solid API on top of which all PMDs and application will be converted.
> > Considering some Holidays and the DPDK summit I won't have much time to
> > work on it.
> > >
> > > The plan is as follows:
> > > 1. complete the last comment on the current series and integrate it.
> > > 2. send a new patchset to convert to the API suggested above.
> > >
> > > Aggregating the different suggestions I come up with the below. if this is
> > agreed, then I will move with the implementation.
> > > (I thought it is good to return error values for the get function).
> >
> > I'd rather you didn't. :-) The only realistic error I would consider is an invalid
> > port id, and I think returning 0 - no offloads - is fine in those cases. The user
> > will pretty quickly discover it's an invalid port id anyway, so I prefer a get
> > function to just return the value as a return value and be done with it!
>
> It would be simpler, however am not sure invalid port is the only error to consider. Another possible error can be the PMD is not supporting this function.
> On that case returning 0 is not good enough. The application cannot know why the offload is not set, is it because it is set wrong or the PMD just don't support this API (which leads me to my next point).
We can skip error reporting on "get" functions
and rely on "set" functions to return error if offload API is not supported
or for other miscellaneous errors.
> Declare:
> API1 = The API being worked so far.
> AP2 = The suggested API being discussed here.
>
> API1 was designed for easy adoption from both PMDs and application. Application can use either old/new API on top of PMD which support one of them thanks to the convert function. There was no hard demand to force all of the PMDs to support it at once.
> With API2 this model breaks. Application which moved to the new offloads API cannot work with PMD which supports the old one.
It means the generic applications cannot migrate to the new API
until every PMDs have migrated.
I don't see it like a big issue.
> If I aggregate the pros for API2:
> From Bruce:
> >* allows some settings to be set before start, and others afterwards,
> > with an appropriate return value if dynamic config not supported.
> >* we can get fine grained error reporting from these - the set calls can
> > all return the mask indicating what offloads could not be applied -
> > zero means all ok, 1 means a problem with that setting. This may be
> > easier for the app to use than feature discovery in some cases.
>
> For that functionality I think the get function are enough (application set offload and then check which of them is actually set).
> The set function are more for the on the fly configuration.
>
> >* for those PMDs which support configuration at a per-queue level, it
> > can allow the user to specify the per-port settings as a default, and
> > then override that value at the queue level, if you just want one queue
> > different from the rest.
>
> This can be done with API1 as well.
>
> From Thomas:
> > make the on the flight vlan offloads configuration more generic.
>
> The cons:
> 1. hard requirement from the PMDs to support the new API.
>
> I can commit for mlx5 and mlx4 PMDs. I know other PMD maintainers plan to convert their PMDs as well. If we take this approach we must make sure they all move.
We can try to get an agreement from more vendors at Dublin summit.
If not, we can wait more than one release cycle for late support.
> There is another possible API (API3):
> 1. keep the per-port, per-queue configuration.
> 2. add the get function for better error reporting and visibility for application.
> 3. keep the current on the flight vlan offloading configuration as an exception. In case there will be a need to configure more offloads on the flight we can move to API2.
>
> With API1 I am obviously OK.
> I agree API2 is more generic.
> API3 is a nice compromise, if we don't want to force all PMD to convert.
The question is: do we want to choose a compromise while breaking this API?
Tuesday, September 19, 2017 12:09 AM, Thomas Monjalon:
> >
> > It would be simpler, however am not sure invalid port is the only error to
> consider. Another possible error can be the PMD is not supporting this
> function.
> > On that case returning 0 is not good enough. The application cannot know
> why the offload is not set, is it because it is set wrong or the PMD just don't
> support this API (which leads me to my next point).
>
> We can skip error reporting on "get" functions and rely on "set" functions to
> return error if offload API is not supported or for other miscellaneous errors.
It will complex the set function then.
Instead of using the return value to understand all offloads were set, the application will need to provide another pointer for the function to understand which offloads were actually set.
I understand this is nice to use the return value of the get without the need of temporary variable, it will save some lines in the code.
But I think that good error reporting to make the application crystal clear why the offloads on get are 0 wins.
> > I can commit for mlx5 and mlx4 PMDs. I know other PMD maintainers plan
> to convert their PMDs as well. If we take this approach we must make sure
> they all move.
>
> We can try to get an agreement from more vendors at Dublin summit.
> If not, we can wait more than one release cycle for late support.
Yes we can discuss on it in Dublin. Still I want to emphasize my concern:
There is no point in moving PMD to the new API if there is no application to use it . Besides of user applications this refers also to testpmd and other examples on dpdk tree (which I plan to convert on 18.02).
PMD maintainers may object to this conversion if their PMD still uses the old offloads APIs.
So can we have guarantee from Thomas/Ferruh that this series, as long it is technically OK, will be accepted? Will we force all that object to change their PMDs?
If not, I think this is bad approach to put the API floating around ethdev with no PMD to implement it.
>
> > There is another possible API (API3):
> > 1. keep the per-port, per-queue configuration.
> > 2. add the get function for better error reporting and visibility for
> application.
> > 3. keep the current on the flight vlan offloading configuration as an
> exception. In case there will be a need to configure more offloads on the
> flight we can move to API2.
> >
> > With API1 I am obviously OK.
> > I agree API2 is more generic.
> > API3 is a nice compromise, if we don't want to force all PMD to convert.
>
> The question is: do we want to choose a compromise while breaking this
> API?
Maybe compromise is not the right word.
We are striving for the generic API2 which has all the full functionality and generalize API1 by supporting on the fly configuration as well.
Maybe for user applications there is no such use-case. How many application decides on the flight to suddenly change the crc strip or the scatter setting ?
Moreover, how many PMDs will actually support such on the flight configuration?
How easy will it be for application to work with the API for PMD which don't support on the flight configuration? They will need to try, and if fail stop the port and try again - in that sense there no much benefit for API2.
Currently we have only the vlan offloads which can be set on the flight and maybe it is more than enough, I don't know, am not familiar with enough application to be sure.
API3 propose to wait with this approach till we will have a proper use case from users.
19/09/2017 09:33, Shahaf Shuler:
> Tuesday, September 19, 2017 12:09 AM, Thomas Monjalon:
> > >
> > > It would be simpler, however am not sure invalid port is the only error to
> > consider. Another possible error can be the PMD is not supporting this
> > function.
> > > On that case returning 0 is not good enough. The application cannot know
> > why the offload is not set, is it because it is set wrong or the PMD just don't
> > support this API (which leads me to my next point).
> >
> > We can skip error reporting on "get" functions and rely on "set" functions to
> > return error if offload API is not supported or for other miscellaneous errors.
>
> It will complex the set function then.
> Instead of using the return value to understand all offloads were set, the application will need to provide another pointer for the function to understand which offloads were actually set.
I think we must forbid setting offloads partially.
If one setting is not possible, nothing should be done.
I don't understand why it would complicate the "set" function.
Anyway, we must report errors in "set" function.
One more question is: do we want to return a mask of "accepted" offload
by getting the mask as a pointer?
> I understand this is nice to use the return value of the get without the need of temporary variable, it will save some lines in the code.
> But I think that good error reporting to make the application crystal clear why the offloads on get are 0 wins.
No strong opinion about error return in "get" function.
It is probably reasonnable to distinguish offload value 0
and "get" function not implemented.
> > > I can commit for mlx5 and mlx4 PMDs. I know other PMD maintainers plan
> > to convert their PMDs as well. If we take this approach we must make sure
> > they all move.
> >
> > We can try to get an agreement from more vendors at Dublin summit.
> > If not, we can wait more than one release cycle for late support.
>
> Yes we can discuss on it in Dublin. Still I want to emphasize my concern:
> There is no point in moving PMD to the new API if there is no application to use it . Besides of user applications this refers also to testpmd and other examples on dpdk tree (which I plan to convert on 18.02).
> PMD maintainers may object to this conversion if their PMD still uses the old offloads APIs.
>
> So can we have guarantee from Thomas/Ferruh that this series, as long it is technically OK, will be accepted? Will we force all that object to change their PMDs?
>
> If not, I think this is bad approach to put the API floating around ethdev with no PMD to implement it.
>
> > > There is another possible API (API3):
> > > 1. keep the per-port, per-queue configuration.
> > > 2. add the get function for better error reporting and visibility for
> > application.
> > > 3. keep the current on the flight vlan offloading configuration as an
> > exception. In case there will be a need to configure more offloads on the
> > flight we can move to API2.
> > >
> > > With API1 I am obviously OK.
> > > I agree API2 is more generic.
> > > API3 is a nice compromise, if we don't want to force all PMD to convert.
> >
> > The question is: do we want to choose a compromise while breaking this
> > API?
>
> Maybe compromise is not the right word.
>
> We are striving for the generic API2 which has all the full functionality and generalize API1 by supporting on the fly configuration as well.
> Maybe for user applications there is no such use-case. How many application decides on the flight to suddenly change the crc strip or the scatter setting ?
> Moreover, how many PMDs will actually support such on the flight configuration?
> How easy will it be for application to work with the API for PMD which don't support on the flight configuration? They will need to try, and if fail stop the port and try again - in that sense there no much benefit for API2.
>
> Currently we have only the vlan offloads which can be set on the flight and maybe it is more than enough, I don't know, am not familiar with enough application to be sure.
>
> API3 propose to wait with this approach till we will have a proper use case from users.
If, as a community, we decide that configuring offloads on the fly
is not a requirement, OK to not plan it in the API.
If we decide to not do it now, we could change again the API later.
Opinions?
@@ -1006,6 +1006,34 @@ rte_eth_dev_close(uint8_t port_id)
dev->data->tx_queues = NULL;
}
+/**
+ * A conversion function from rxmode offloads API to rte_eth_rxq_conf
+ * offloads API.
+ */
+static void
+rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
+ struct rte_eth_rxq_conf *rxq_conf)
+{
+ if (rxmode->header_split == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
+ if (rxmode->hw_ip_checksum == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
+ if (rxmode->hw_vlan_filter == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
+ if (rxmode->hw_vlan_strip == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
+ if (rxmode->hw_vlan_extend == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
+ if (rxmode->jumbo_frame == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+ if (rxmode->hw_strip_crc == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
+ if (rxmode->enable_scatter == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
+ if (rxmode->enable_lro == 1)
+ rxq_conf->offloads |= DEV_RX_OFFLOAD_TCP_LRO;
+}
+
int
rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
uint16_t nb_rx_desc, unsigned int socket_id,
@@ -1016,6 +1044,8 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
uint32_t mbp_buf_size;
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
+ struct rte_eth_rxq_conf rxq_trans_conf;
+ /* Holds translated configuration to be passed to the PMD */
void **rxq;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1062,6 +1092,11 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
return -EINVAL;
}
+ if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
+ (dev->data->dev_conf.rxmode.ignore_offloads == 1)) {
+ return -ENOTSUP;
+ }
+
if (nb_rx_desc > dev_info.rx_desc_lim.nb_max ||
nb_rx_desc < dev_info.rx_desc_lim.nb_min ||
nb_rx_desc % dev_info.rx_desc_lim.nb_align != 0) {
@@ -1086,8 +1121,15 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
if (rx_conf == NULL)
rx_conf = &dev_info.default_rxconf;
+ rxq_trans_conf = *rx_conf;
+ if ((dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD) &&
+ (dev->data->dev_conf.rxmode.ignore_offloads == 0)) {
+ rte_eth_convert_rxmode_offloads(&dev->data->dev_conf.rxmode,
+ &rxq_trans_conf);
+ }
+
ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
- socket_id, rx_conf, mp);
+ socket_id, &rxq_trans_conf, mp);
if (!ret) {
if (!dev->data->min_rx_buf_size ||
dev->data->min_rx_buf_size > mbp_buf_size)
@@ -1097,6 +1139,49 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
return ret;
}
+/**
+ * A conversion function from txq_flags to rte_eth_txq_conf offloads API.
+ */
+static void
+rte_eth_convert_txq_flags(struct rte_eth_txq_conf *txq_conf)
+{
+ uint32_t txq_flags = txq_conf->txq_flags;
+ uint64_t *offloads = &txq_conf->offloads;
+
+ if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
+ *offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
+ if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
+ *offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
+ if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
+ *offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
+ if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
+ *offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
+ if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
+ *offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
+}
+
+/**
+ * A conversion function between rte_eth_txq_conf offloads API to txq_flags
+ * offloads API.
+ */
+static void
+rte_eth_convert_txq_offloads(struct rte_eth_txq_conf *txq_conf)
+{
+ uint32_t *txq_flags = &txq_conf->txq_flags;
+ uint64_t offloads = txq_conf->offloads;
+
+ if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
+ *txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
+ if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
+ *txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
+ if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
+ *txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
+ if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
+ *txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
+ if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
+ *txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP;
+}
+
int
rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
uint16_t nb_tx_desc, unsigned int socket_id,
@@ -1104,6 +1189,8 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
{
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
+ struct rte_eth_txq_conf txq_trans_conf;
+ /* Holds translated configuration to be passed to the PMD */
void **txq;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1148,8 +1235,16 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
if (tx_conf == NULL)
tx_conf = &dev_info.default_txconf;
+ txq_trans_conf = *tx_conf;
+ if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
+ (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
+ rte_eth_convert_txq_flags(&txq_trans_conf);
+ else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
+ (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
+ rte_eth_convert_txq_offloads(&txq_trans_conf);
+
return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id, nb_tx_desc,
- socket_id, tx_conf);
+ socket_id, &txq_trans_conf);
}
void