Re: [dpdk-dev] [PATCH v3] ether: use a default for max Rx frame size in configure()

[Shahaf Shuler <shahafs@mellanox.com>]

Hi Andriy,

I think this patch addressing just small issue in a bigger problem.
The way I see it all application needs to specify is the max packet size it expects to receive, nothing else(!). 
Currently we are forcing it to toggle multiple redundant fields. 

Wednesday, May 23, 2018 1:31 AM, Thomas Monjalon:
> Subject: Re: [dpdk-dev] [PATCH v3] ether: use a default for max Rx frame
> > >
> > > IMO the changes are transparent for the PMDs (please see below), but
> > > it might affect some applications. Here is the change in API behaviour:
> > >
> > > Before the patch:
> > > jumbo == 0, max_rx_pkt_len == 0, RESULT: max_rx_pkt_len =
> > > ETHER_MAX_LEN jumbo == 0, max_rx_pkt_len == 10, RESULT:
> > > max_rx_pkt_len = ETHER_MAX_LEN jumbo == 0, max_rx_pkt_len ==
> 1200,
> > > RESULT: max_rx_pkt_len = 1200 jumbo == 0, max_rx_pkt_len == 9K,
> > > RESULT: max_rx_pkt_len = ETHER_MAX_LEN
> > >
> > > jumbo == 1, max_rx_pkt_len == 0, RESULT: ERROR jumbo == 1,
> > > max_rx_pkt_len == 10, RESULT: ERROR jumbo == 1, max_rx_pkt_len ==
> > > 1200, RESULT: max_rx_pkt_len = 1200 jumbo == 1, max_rx_pkt_len ==
> > > 9K, RESULT: ERROR or max_rx_pkt_len = 9K jumbo == 1, max_rx_pkt_len
> > > == 90K, RESULT: ERROR
> > >
> > >
> > > After the patch:
> > > jumbo == 0, max_rx_pkt_len == 0, RESULT: max_rx_pkt_len =
> > > ETHER_MAX_LEN jumbo == 0, max_rx_pkt_len == 10, RESULT: ERROR
> > > (changed) jumbo == 0, max_rx_pkt_len == 1200, RESULT:
> max_rx_pkt_len
> > > = 1200 jumbo == 0, max_rx_pkt_len == 9K, RESULT: ERROR (changed)

For example in the line above - the application wants to receive 9K frames. Why it is an error? Why forcing the application to set another redundant bit?
IMO The "jumbo_frame" bit can be set by the underlying PMD directly to the device registers given the max_rx_pkt_len configuration. 

Same apply to DEV_RX_OFFLOAD_SCATTER flag. Application doesn't need to set it. the PMD will choose the put multiple mbufs in Rx descriptor given the mbuf size and the max_rx_packet_len. 
API should be informative enough for the application to expect multi-segment packets on the receive. 

Finally the MTU. It is correct it stands for "max transmit unit" but it is threaded in the same way max_rx_pkt_len is (i.e. MTU sets the max packet len the port receives). 
We need to either clarify it only address transmit or remove it completely and maybe change max_rx_pkt_len name to mtu. 

> > >
> > > jumbo == 1, max_rx_pkt_len == 0, RESULT: max_rx_pkt_len = dev_info()
> > > jumbo == 1, max_rx_pkt_len == 10, RESULT: ERROR jumbo == 1,
> > > max_rx_pkt_len == 1200, RESULT: max_rx_pkt_len = 1200 jumbo == 1,
> > > max_rx_pkt_len == 9K, RESULT: ERROR or max_rx_pkt_len = 9K jumbo
> ==
> > > 1, max_rx_pkt_len == 90K, RESULT: ERROR
> > >
> > > Only the apps which requested too small or too big normal frames
> > > will be affected. In most cases it will be rather an error in the app...
> > >
> > >
> > > Also I have looked through all the PMDs to confirm they are not
> > > affected. Here is the summary:
> > >
> > > af_packet
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = ETH_FRAME_LEN (1514)
> > >
> > > ark
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = ETH_FRAME_LEN (16K - 128)
> > >
> > > avp
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = avp->max_rx_pkt_len
> > > rx_queue_setup() uses max_rx_pkt_len for scattering
> > >
> > > bnx2x
> > > configure() uses max_rx_pkt_len to set internal mtu
> > > info() returns max_rx_pktlen = BNX2X_MAX_RX_PKT_LEN (15872)
> > >
> > > bnxt
> > > configure() uses max_rx_pkt_len to set internal mtu
> > > info() returns max_rx_pktlen = BNXT_MAX_MTU + ETHER_HDR_LEN +
> > > ETHER_CRC_LEN + VLAN_TAG_SIZE (9000 + 14 + 4 + 4)
> > >
> > > bonding
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = internals->candidate_max_rx_pktlen or
> > > ETHER_MAX_JUMBO_FRAME_LEN (0x3F00)
> > >
> > > cxgbe
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = CXGBE_MAX_RX_PKTLEN (9000 + 14 + 4)
> > > rx_queue_setup() checks max_rx_pkt_len boundaries
> > >
> > > dpaa2
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = DPAA2_MAX_RX_PKT_LEN (10240)
> > >
> > > e1000 (em)
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = em_get_max_pktlen() (0x2412, 0x1000,
> > > 1518, 0x3f00, depends on model)
> > >
> > > e1000 (igb)
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = 0x3fff
> > > start() writes max_rx_pkt_len to HW for jumbo frames only
> > > start() uses max_rx_pkt_len for scattering
> > >
> > > ena
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = adapter->max_mtu
> > > start() checks max_rx_pkt_len boundaries
> > >
> > > enic
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = enic->max_mtu + 14 + 4
> > >
> > > fm10k
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = FM10K_MAX_PKT_SIZE (15 * 1024)
> > > start() uses max_rx_pkt_len for scattering
> > >
> > > i40e
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = I40E_FRAME_SIZE_MAX (9728)
> > > rx_queue_config() checks max_rx_pkt_len boundaries
> > >
> > > ixgbe
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = 15872 (9728 for vf)
> > > start() writes max_rx_pkt_len to HW for jumbo frames only
> > > start() uses max_rx_pkt_len for scattering
> > >
> > > kni
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = UINT32_MAX
> > >
> > > liquidio
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = LIO_MAX_RX_PKTLEN (64K)
> > > start() checks max_rx_pkt_len boundaries
> > >
> > > mlx4
> > > configure() uses max_rx_pkt_len for scattering
> > > info() returns max_rx_pktlen = 65536
> > >
> > > mlx5
> > > configure() uses max_rx_pkt_len for scattering
> > > info() returns max_rx_pktlen = 65536
> > >
> > > nfp
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = hw->mtu
> > >
> > > null
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = (uint32_t)-1
> > >
> > > pcap
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = (uint32_t)-1
> > >
> > > qede
> > > configure() uses max_rx_pkt_len for scattering + internal data
> > > info() returns max_rx_pktlen = ETH_TX_MAX_NON_LSO_PKT_LEN (9700
> - 4
> > > - 4
> > > - 12 - 8)
> > >
> > > ring
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = (uint32_t)-1
> > >
> > > sfc
> > > configure() uses max_rx_pkt_len to set internal data
> > > info() returns max_rx_pktlen = EFX_MAC_PDU_MAX (9202 + 14 + 4 + 4 +
> > > 16)
> > >
> > > szedata2
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = (uint32_t)-1
> > >
> > > tap
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = ETHER_MAX_VLAN_FRAME_LEN (1518 +
> 4)
> > >
> > > thunderx
> > > configure() uses max_rx_pkt_len for scattering + sets internal mtu
> > > info() returns max_rx_pktlen = NIC_HW_MAX_FRS (9200)
> > >
> > > vhost
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = (uint32_t)-1
> > >
> > > virtio
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = VIRTIO_MAX_RX_PKTLEN (9728U)
> > >
> > > vmxnet3
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = 16384;
> > >
> > > xenvirt
> > > configure() does not use max_rx_pkt_len
> > > info() returns max_rx_pktlen = 2048
> > >
> > >
> > > Regards,
> > > Andriy
> > >
> >
> >
> >
> >
> 
> 
> 
>