On Fri, Nov 01, 2024 at 02:42:20PM +0100, Martin Weiser wrote:
> Hi Bruce,
>
> thank you very much for your feedback.
> Please see my answers inline below.
>
> I will send a v2 of the patch.
>
> Best regards,
> Martin
>
>
> Am 29.10.24 um 18:42 schrieb Bruce Richardson:
> > On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote:
> >>
> >> The issue only appeared with hardware-timestamping enabled
> >> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
> >>
> >> The length of the prepended hardware timestamp was not subtracted from
> >> the data length so that received packets were 16 bytes longer than
> >> expected.
> >>
> >> In scatter-gather mode only the first mbuf has a timestamp but the
> >> data offset of the follow-up mbufs was not adjusted accordingly.
> >> This caused 16 bytes of packet data to be missing between
> >> the segments.
> >>
> >> Signed-off-by: Martin Weiser <martin.wei...@allegro-packets.com>
> >> ---
> >
> > Hi,
> >
> > thanks for the patch. Some comments inline below.
> >
> > /Bruce
> >
> >> drivers/net/igc/igc_txrx.c | 9 +++++++++
> >> 1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
> >> index d0cee1b016..2fafa91bd5 100644
> >> --- a/drivers/net/igc/igc_txrx.c
> >> +++ b/drivers/net/igc/igc_txrx.c
> >> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf
> >> **rx_pkts, uint16_t nb_pkts)
> >>
> >> rxm->data_off = RTE_PKTMBUF_HEADROOM;
> >> data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
> >> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
> >> + data_len -= IGC_TS_HDR_LEN;
> >> rxm->data_len = data_len;
> >> rxm->pkt_len = data_len;
> >> rxm->nb_segs = 1;
> >> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct
> >> rte_mbuf **rx_pkts,
> >> */
> >> rxm->data_off = RTE_PKTMBUF_HEADROOM;
> >> data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
> >> + if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
> >> + if (first_seg == NULL)
> >> + data_len -= IGC_TS_HDR_LEN;
> >> + else
> >> + rxm->data_off -= IGC_TS_HDR_LEN;
> >
> > This initially confused me, because I was assuming that data_off was a typo
> > for data_len. However, then I realised on closer examination that, when
> > timestamp offload is enabled, the actual buffer addresses sent down to the
> > hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt
> > has the start of data at "normal" data_offset in mbuf, but subsequent
> > buffers need adjustment). Is my understanding of the issue correct?
>
> This is exactly right. Took me a bit to understand what was happening here and
> I do not know if there might be a better way do this than to change the start
> offset of all descriptors. But there is probably a reason why it was done
> this way.
> Initially the issue was detected as forwarding of packets that maxed out the
> MTU
> failed since they now, with the increased length, exceeded the MTU. Only then
> it
> became apparent that the scatter-gather handling was also broken.
>
> >
> > In either case, I have two small bits of feedback on this:
> > * Firstly, I think this needs a comment explaining the logic here, to avoid
> > others being confused as I was.
>
> This will be part of patch v2.
>
> > * Secondly, a very minor point, but is the code clearer or shorter, if you
> > merge this extra code down into the next block which is already checking
> > for the first_segment of a packet or not?
>
> I would actually prefer to keep it that way as not to mix up the the data
> buffer
> address and length handling with the mbuf chain construction.
>
Ok, that is fine if you think it better. I'll just expect V2 with some
added comments to clarify things.
Thanks,
/Bruce
