Re: [dpdk-dev] [PATCH] mbuf: fix for incomplete nb_segs types change

[Olivier MATZ <olivier.matz@6wind.com>]

On Mon, Nov 13, 2017 at 02:18:55PM +0400, Ilya Matveychikov wrote:
> 
> > On Nov 13, 2017, at 2:10 PM, Olivier MATZ <olivier.matz@6wind.com> wrote:
> > 
> > Hi Ilya,
> > 
> > On Fri, Nov 10, 2017 at 04:56:43PM +0300, Ilya V. Matveychikov wrote:
> >> Update types of variables to correspond to nb_segs type change from
> >> uint8_t to uint16_t.
> >> 
> >> Fixes: 97cb466d65c9 ("mbuf: use 2 bytes for port and nb segments")
> >> Cc: olivier.matz@6wind.com
> >> 
> >> Signed-off-by: Ilya V. Matveychikov <matvejchikov@gmail.com>
> > 
> > Thanks for this fix.
> > 
> > I did quick search for other references:
> > 
> > $ git grep seg | grep -i int8
> > app/test-pmd/config.c:  tx_pkt_nb_segs = (uint8_t) nb_segs;
> > app/test-pmd/csumonly.c:        uint16_t seglen[], uint8_t nb_seg)
> > app/test-pmd/testpmd.c:uint8_t  tx_pkt_nb_segs = 1; /**< Number of segments in TXONLY packets */
> > app/test-pmd/testpmd.h:extern uint8_t  tx_pkt_nb_segs; /**< Number of segments in TX packets */
> > doc/guides/sample_app_ug/ipv4_multicast.rst:        hdr->pkt.nb_segs = (uint8_t)(pkt->pkt.nb_segs + 1);
> > drivers/bus/dpaa/rte_dpaa_bus.h:                        return (uint8_t *)(memseg[i].addr) +
> > drivers/net/ark/ark_ethdev_rx.c:        uint8_t segments;
> > drivers/net/avp/rte_avp_common.h:       uint8_t nb_segs; /**< Number of segments */
> > drivers/net/bnx2x/bnx2x.c:bnx2x_igu_ack_sb(struct bnx2x_softc *sc, uint8_t igu_sb_id, uint8_t segment,
> > drivers/net/bnx2x/bnx2x.c:              uint8_t segment;
> > drivers/net/bnx2x/bnx2x.h:bnx2x_igu_ack_sb_gen(struct bnx2x_softc *sc, uint8_t segment,
> > drivers/net/bnx2x/ecore_hsi.h:  uint8_t igu_seg_id;
> > drivers/net/bnx2x/ecore_hsi.h:  uint8_t igu_seg_id;
> > drivers/net/bnx2x/ecore_hsi.h:  uint8_t igu_seg_id /* segment id of the IGU */;
> > drivers/net/bnx2x/ecore_hsi.h:  uint8_t igu_seg_id /* segment id of the IGU */;
> > drivers/net/bnxt/hsi_struct_def_dpdk.h: uint8_t tpa_segs;
> > drivers/net/dpaa2/dpaa2_rxtx.c: first_seg->buf_addr = (uint8_t *)sg_addr;
> > drivers/net/dpaa2/dpaa2_rxtx.c:         next_seg->buf_addr  = (uint8_t *)sg_addr;
> > drivers/net/i40e/i40e_rxtx.c:            * m->nb_segs is uint8_t, so nb_segs is always less than
> > drivers/net/ixgbe/ixgbe_rxtx.c: int8_t i, nb_segs = m->nb_segs;
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:txq_wr_dseg_v(struct mlx5_txq_data *txq, uint8_t *dseg,
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:  const uint8x16_t dseg_shuf_m = {
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:          uint8_t *dseg;
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:          dseg = (uint8_t *)(wqe + 1);
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:                          dseg = (uint8_t *)
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:  uint8_t *dseg;
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:  dseg = (uint8_t *)(wqe + 1);
> > drivers/net/mlx5/mlx5_rxtx_vec_neon.h:          dseg = (uint8_t *)txq->wqes;
> > drivers/net/qede/qede_rxtx.c:                uint8_t num_segs, uint16_t pkt_len)
> > drivers/net/qede/qede_rxtx.c:   uint8_t num_segs = 1;
> > drivers/net/qede/qede_rxtx.c:   uint8_t nb_segs = 0;
> > drivers/net/sfc/base/ef10_nvram.c:      if ((rc = tlv_init_cursor_from_size(&cursor, (uint8_t *)seg_data,
> > drivers/net/sfc/base/ef10_nvram.c:      if ((rc = tlv_init_cursor_from_size(&cursor, (uint8_t *)seg_data,
> > drivers/net/sfc/base/ef10_nvram.c:      if ((rc = tlv_init_cursor_from_size(&cursor, (uint8_t *)seg_data,
> > drivers/net/sfc/base/ef10_nvram.c:      if ((rc = tlv_init_cursor_from_size(&cursor, (uint8_t *)seg_data,
> > drivers/net/szedata2/rte_eth_szedata2.c:        uint8_t mbuf_segs;
> > drivers/net/vmxnet3/base/vmxnet3_defs.h:   uint8  segCnt;       /* Number of aggregated packets */
> > examples/ipv4_multicast/main.c: hdr->nb_segs = (uint8_t)(pkt->nb_segs + 1);
> > lib/librte_mbuf/rte_mbuf.h:     uint8_t nseg;
> > lib/librte_mbuf/rte_mbuf.h:     head->nb_segs = (uint8_t)(head->nb_segs + tail->nb_segs);
> > lib/librte_pdump/rte_pdump.c:   uint8_t nseg;
> > test/test/packet_burst_generator.c:             uint8_t pkt_len, uint8_t nb_pkt_segs)
> > test/test/packet_burst_generator.c:             int nb_pkt_per_burst, uint8_t pkt_len, uint8_t nb_pkt_segs)
> > test/test/packet_burst_generator.h:             uint8_t pkt_len, uint8_t nb_pkt_segs);
> > test/test/packet_burst_generator.h:             int nb_pkt_per_burst, uint8_t pkt_len, uint8_t nb_pkt_segs);
> > test/test/test_cryptodev.h:             int nb_segs, uint8_t pattern) {
> > 
> > I think the ones in ixgbe, szedata and examples/ipv4_multicast/main.c could be
> > fixed without risk.
> > Some don't look critical for now (ex: testpmd), but could be integrated in this
> > patch too.
> > 
> 
> Something already fixed by:
> 6c293ffd63b3f61a95d8c492d007160874e7a057
> 4c20622a95087b42ddb403406d3877a9dc3d731b

You are right, sorry, I didn't update my workspace since rc3.

> But I didn’t try to fix it everywhere in drivers as it may depends of the particular hardware - does it support more that uint8_t queues or not. So, I agree with you that maintainers should have a look on that change.
> 
> Also, not sure that testpmd need to have more that 255 segments of 16k possible. And my opinion that it’s better to apply this patch separately without integration with drivers fixes and stuff like that.

Agree.

Acked-by: Olivier Matz <olivier.matz@6wind.com>