Hi Radu,
Monday, December 11, 2017 1:48 PM, Radu Nicolau :
> Hi,
>
> Comment inline
>
>
> On 11/23/2017 12:19 PM, Shahaf Shuler wrote:
> > Ethdev offloads API has changed since:
> >
> > commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") commit
> > cba7f53b717d ("ethdev: introduce Tx queue offloads API")
> >
> > This commit support the new API.
> >
> > Signed-off-by: Shahaf Shuler <shah...@mellanox.com>
> > ---
> > examples/ipsec-secgw/ipsec-secgw.c | 27
> +++++++++++++++++++++++++--
> > 1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/ipsec-secgw.c
> > b/examples/ipsec-secgw/ipsec-secgw.c
> > index c98454a90..6e538a1ab 100644
> > --- a/examples/ipsec-secgw/ipsec-secgw.c
> > +++ b/examples/ipsec-secgw/ipsec-secgw.c
> > @@ -217,6 +217,8 @@ static struct rte_eth_conf port_conf = {
> > },
> > .txmode = {
> > .mq_mode = ETH_MQ_TX_NONE,
> > + .offloads = (DEV_TX_OFFLOAD_IPV4_CKSUM |
> > + DEV_TX_OFFLOAD_MULTI_SEGS),
> > },
> > };
> >
> > @@ -1394,6 +1396,22 @@ port_init(uint16_t portid)
> > if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_SECURITY)
> > port_conf.txmode.offloads |= DEV_TX_OFFLOAD_SECURITY;
> >
> > + if ((dev_info.rx_offload_capa & port_conf.rxmode.offloads) !=
> > + port_conf.rxmode.offloads) {
> > + printf("Some Rx offloads are not supported "
> > + "by port %d: requested 0x%lx supported 0x%lx\n",
> > + portid, port_conf.rxmode.offloads,
> > + dev_info.rx_offload_capa);
> > + port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> > + }
> > + if ((dev_info.tx_offload_capa & port_conf.txmode.offloads) !=
> > + port_conf.txmode.offloads) {
> > + printf("Some Tx offloads are not supported "
> > + "by port %d: requested 0x%lx supported 0x%lx\n",
> > + portid, port_conf.txmode.offloads,
> > + dev_info.tx_offload_capa);
> > + port_conf.txmode.offloads &= dev_info.tx_offload_capa;
> > + }
> I don't think that clearing the offload flags that are not advertised in the
> capabilities is a good approach, although it may be the right one.
> From what I can see there are more PMDs that don't fully populate the
> offload capabilities, but actually check for them in the configure/start
> function. One of them is ixgbe, which needs CRC strip enabled when IPSec is
> enabled, and will fail to start otherwise. So although it supports CRC strip
> it
> does not set the flag in the capabilities, but checks it in the start
> function.
Why ixgbe don't expose the CRC cap then? It seems wrong behavior to expect the
application to set it without any cap reported.
> I would propose to just print a warning if a requested offload is not set in
> the
> capabilities, but let the pmd start fail if it is not really supported.
I think I agree, however not from the reason you mentioned.
It is bad to mask the un-supported offloads because the application relies on
them to be set successfully. The application will not run successfully if the
IPV4 checksum is not actually set (for example).
On v2 I will print just the warn.
>
> > ret = rte_eth_dev_configure(portid, nb_rx_queue, nb_tx_queue,
> > &port_conf);
> > if (ret < 0)
> > @@ -1420,7 +1438,8 @@ port_init(uint16_t portid)
> > printf("Setup txq=%u,%d,%d\n", lcore_id, tx_queueid,
> socket_id);
> >
> > txconf = &dev_info.default_txconf;
> > - txconf->txq_flags = 0;
> > + txconf->txq_flags = ETH_TXQ_FLAGS_IGNORE;
> > + txconf->offloads = port_conf.txmode.offloads;
> >
> > ret = rte_eth_tx_queue_setup(portid, tx_queueid, nb_txd,
> > socket_id, txconf);
> > @@ -1434,6 +1453,8 @@ port_init(uint16_t portid)
> >
> > /* init RX queues */
> > for (queue = 0; queue < qconf->nb_rx_queue; ++queue) {
> > + struct rte_eth_rxconf rxq_conf;
> > +
> > if (portid != qconf->rx_queue_list[queue].port_id)
> > continue;
> >
> > @@ -1442,8 +1463,10 @@ port_init(uint16_t portid)
> > printf("Setup rxq=%d,%d,%d\n", portid, rx_queueid,
> > socket_id);
> >
> > + rxq_conf = dev_info.default_rxconf;
> > + rxq_conf.offloads = port_conf.rxmode.offloads;
> > ret = rte_eth_rx_queue_setup(portid, rx_queueid,
> > - nb_rxd, socket_id, NULL,
> > + nb_rxd, socket_id,
> &rxq_conf,
> > socket_ctx[socket_id].mbuf_pool);
> > if (ret < 0)
> > rte_exit(EXIT_FAILURE,
