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.yi...@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 <pogonarumi...@gmail.com> > > Signed-off-by: Tudor Cornea <tudor.cor...@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 */ > > > >
