[dpdk-dev] [PATCH v2] net/af_packet: remove timestamp from packet status
Tudor Cornea
tudor.cornea at gmail.com
Tue Sep 21 23:02:37 CEST 2021
Thanks for the suggestion. I will send a new version of the patch with the
required changes.
Tudor
On Mon, 20 Sept 2021 at 20:49, Ferruh Yigit <ferruh.yigit at intel.com> wrote:
> On 9/13/2021 6:23 PM, Tudor Cornea wrote:
> > We should eliminate the timestamp status from the packet
> > status. This should only matter if timestamping is enabled
> > on the socket, but we might hit a kernel bug, which is fixed
> > in newer releases.
> >
> > For interfaces of type 'veth', the sent skb is forwarded
> > to the peer and back into the network stack which timestamps
> > it on the RX path if timestamping is enabled globally
> > (which happens if any socket enables timestamping).
> >
> > When the skb is destructed, tpacket_destruct_skb() is called
> > and it calls __packet_set_timestamp() which doesn't check
> > the flags on the socket and returns the timestamp if it is
> > set in the skb (and for veth it is, as mentioned above).
> >
> > See the following kernel commit for reference [1]:
> >
> > net: packetmmap: fix only tx timestamp on request
> >
> > The packetmmap tx ring should only return timestamps if requested
> > via setsockopt PACKET_TIMESTAMP, as documented. This allows
> > compatibility with non-timestamp aware user-space code which checks
> > tp_status == TP_STATUS_AVAILABLE; not expecting additional timestamp
> > flags to be set in tp_status.
> >
> > [1] https://www.spinics.net/lists/kernel/msg3959391.html
> >
> > Signed-off-by: Mihai Pogonaru <pogonarumihai at gmail.com>
> > Signed-off-by: Tudor Cornea <tudor.cornea at gmail.com>
> >
> > ---
> > v2:
> > * Remove compile-time check for kernel version
>
> OK, Stephen's comment makes sense.
>
> > ---
> > drivers/net/af_packet/rte_eth_af_packet.c | 20 ++++++++++++++++++--
> > 1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
> b/drivers/net/af_packet/rte_eth_af_packet.c
> > index b73b211..7ecea4e 100644
> > --- a/drivers/net/af_packet/rte_eth_af_packet.c
> > +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> > @@ -167,6 +167,22 @@ eth_af_packet_rx(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> > return num_rx;
> > }
> >
> > +static inline bool tx_ring_status_unavailable(uint32_t tp_status)
> > +{
>
> Minor syntax comment, can you have the 'static inline bool' part in
> separate
> line. And a basic function comment can be good.
>
> Thanks,
> ferruh
>
> > + /*
> > + * We eliminate the timestamp status from the packet status.
> > + * This should only matter if timestamping is enabled on the
> socket,
> > + * but there is a bug in the kernel which is fixed in newer
> releases.
> > + *
> > + * See the following kernel commit for reference:
> > + * commit 171c3b151118a2fe0fc1e2a9d1b5a1570cfe82d2
> > + * net: packetmmap: fix only tx timestamp on request
> > + */
> > + tp_status &= ~(TP_STATUS_TS_SOFTWARE | TP_STATUS_TS_RAW_HARDWARE);
> > +
> > + return tp_status != TP_STATUS_AVAILABLE;
> > +}
> > +
> > /*
> > * Callback to handle sending packets through a real NIC.
> > */
> > @@ -212,8 +228,8 @@ eth_af_packet_tx(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> > }
> >
> > /* point at the next incoming frame */
> > - if ((ppd->tp_status != TP_STATUS_AVAILABLE) &&
> > - (poll(&pfd, 1, -1) < 0))
> > + if (tx_ring_status_unavailable(ppd->tp_status) &&
> > + poll(&pfd, 1, -1) < 0)
> > break;
> >
> > /* copy the tx frame data */
> >
>
>
More information about the dev
mailing list