04/07/2018 13:16, Andrew Rybchenko:
> On 07/03/2018 12:27 AM, Thomas Monjalon wrote:
> > --- a/doc/guides/sample_app_ug/link_status_intr.rst
> > +++ b/doc/guides/sample_app_ug/link_status_intr.rst
> > @@ -137,10 +137,7 @@ The global configuration is stored in a static
> > structure:
> > static const struct rte_eth_conf port_conf = {
> > .rxmode = {
> > .split_hdr_size = 0,
> > - .header_split = 0, /**< Header Split disabled */
> > - .hw_ip_checksum = 0, /**< IP checksum offload disabled */
> > - .hw_vlan_filter = 0, /**< VLAN filtering disabled */
> > - .hw_strip_crc= 0, /**< CRC stripped by hardware */
> > + .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>
> Is it intended that CRC strip was disabled before and now it is becoming
> enabled?
Yes, I consider the comment to be the real intent.
> > --- a/examples/bbdev_app/main.c
> > +++ b/examples/bbdev_app/main.c
> > @@ -64,11 +64,7 @@ static const struct rte_eth_conf port_conf = {
> > .mq_mode = ETH_MQ_RX_NONE,
> > .max_rx_pkt_len = ETHER_MAX_LEN,
> > .split_hdr_size = 0,
> > - .header_split = 0, /**< Header Split disabled */
> > - .hw_ip_checksum = 0, /**< IP checksum offload disabled */
> > - .hw_vlan_filter = 0, /**< VLAN filtering disabled */
> > - .jumbo_frame = 0, /**< Jumbo Frame Support disabled */
> > - .hw_strip_crc = 0, /**< CRC stripped by hardware */
> > + .offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>
> Is it intended that CRC strip was disabled before and now it is becoming
> enabled?
Yes, I consider the comment to be the real intent.
> > --- a/test/test/test_pmd_perf.c
> > +++ b/test/test/test_pmd_perf.c
> > @@ -97,11 +90,6 @@ static struct rte_eth_txconf tx_conf = {
> > },
> > .tx_free_thresh = 32, /* Use PMD default values */
> > .tx_rs_thresh = 32, /* Use PMD default values */
> > - .txq_flags = (ETH_TXQ_FLAGS_NOMULTSEGS |
> > - ETH_TXQ_FLAGS_NOVLANOFFL |
> > - ETH_TXQ_FLAGS_NOXSUMSCTP |
> > - ETH_TXQ_FLAGS_NOXSUMUDP |
> > - ETH_TXQ_FLAGS_NOXSUMTCP)
> > };
> >
> > enum {
> > @@ -808,38 +796,29 @@ test_set_rxtx_conf(cmdline_fixed_string_t mode)
> >
> > if (!strcmp(mode, "vector")) {
> > /* vector rx, tx */
> > - tx_conf.txq_flags = 0xf01;
>
> I'd say that 100% correct equivalent would be:
> tx_conf.offloads &= ~(DEV_TX_OFFLOAD_VLAN_INSERT |
> DEV_TX_OFFLOAD_IPV4_CKSUM | DEV_TX_OFFLOAD_UDP_CKSUM |
> DEV_TX_OFFLOAD_TCP_CKSUM | DEV_TX_OFFLOAD_SCTP_CKSUM |
> DEV_TX_OFFLOAD_MULTI_SEGS);
I'd say it is a really crappy code, and probably tuned for Intel devices only.
>
> I guess the function may be called few times with different mode set.
> If so, similar fixes should be applied below as well.
>
> > tx_conf.tx_rs_thresh = 32;
> > tx_conf.tx_free_thresh = 32;
> > - port_conf.rxmode.hw_ip_checksum = 0;
> > - port_conf.rxmode.enable_scatter = 0;
>
> port_conf.rxmode.offloads &= ~(DEV_RX_OFFLOAD_CHECKSUM |
> DEV_RX_OFFLOAD_SCATTER);
>
> > return 0;
> > } else if (!strcmp(mode, "scalar")) {
> > /* bulk alloc rx, full-featured tx */
> > - tx_conf.txq_flags = 0;
>
> I think here we should enable offloads listed above to have
> full-featured Tx:
> tx_conf.offloads |= ...
>
> > tx_conf.tx_rs_thresh = 32;
> > tx_conf.tx_free_thresh = 32;
> > - port_conf.rxmode.hw_ip_checksum = 1;
> > - port_conf.rxmode.enable_scatter = 0;
> > + port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;
>
> port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_SCATTER;
>
> > return 0;
> > } else if (!strcmp(mode, "hybrid")) {
> > /* bulk alloc rx, vector tx
> > * when vec macro not define,
> > * using the same rx/tx as scalar
> > */
> > - tx_conf.txq_flags = 0xf01;
>
> As in similar case above.
>
> > tx_conf.tx_rs_thresh = 32;
> > tx_conf.tx_free_thresh = 32;
> > - port_conf.rxmode.hw_ip_checksum = 1;
> > - port_conf.rxmode.enable_scatter = 0;
> > + port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;
>
> As in similar case above
>
> > return 0;
> > } else if (!strcmp(mode, "full")) {
> > /* full feature rx,tx pair */
> > - tx_conf.txq_flags = 0x0; /* must condition */
>
> As in similar case above.
>
> > tx_conf.tx_rs_thresh = 32;
> > tx_conf.tx_free_thresh = 32;
> > - port_conf.rxmode.hw_ip_checksum = 0;
> > - port_conf.rxmode.enable_scatter = 1; /* must condition */
> > + port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER;
>
> port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_CHECKSUM;
>
> > return 0;
> > }
> >
>
> In general I think that it would be really good to avoid changes in
> behaviour when technical changes are done.
I agree, but in this case, it is impossible to know what was the real intent.
And I am perfectly fine breaking bad code.
The other option is to just remove the file. Maybe the best option?
