From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 92E0AA0C45; Tue, 21 Sep 2021 23:02:51 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 57FFF4003F; Tue, 21 Sep 2021 23:02:50 +0200 (CEST) Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by mails.dpdk.org (Postfix) with ESMTP id 04B844003C for ; Tue, 21 Sep 2021 23:02:48 +0200 (CEST) Received: by mail-ed1-f41.google.com with SMTP id r5so1318461edi.10 for ; Tue, 21 Sep 2021 14:02:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=cSBrvXt3Rrjqlk8UQgEMUvLCN4lrire5Uy2HEq5iIY8=; b=lG08gXk1zXtIVBhbgefFq+4LhbbN+qTqiXW7CyxSCWeW7c7S/gOYZg3jodv4ezyuUC 0Q+ZSl/Au3Yr5IgjOjgg7kI1m+bMP/Dhn+vF2oCppox/0T5tzyqnZ3d/QiFUpbRE4qgx cLj3PCnCARfoZp04lukC3q309mO3kLva7reEz3wEXLjjwW+vXhwZdCTIyAbQPslCkVsA Yb1hRzoL+b7KjZp5xHZASAHFsIzsKpQ3iaWbpRwB4hRYFExTj/Gt1J9033Srt2DTvNIN FtMAMXPuSb6H8iZqX54Y/VETeGqG+Hb4MDtw7fd37z9igHUkHO0/WMHmQGHunAchU9if h/4g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=cSBrvXt3Rrjqlk8UQgEMUvLCN4lrire5Uy2HEq5iIY8=; b=0cQ1e7pq4tUL3UaZFpUD8UO4aCovLV8t9W/g9uE9rP6DDdRw1/bQ6ZF4vG+dKjUcy4 Wf3AELPMA6O3OXh9htHPQ4dfwKilmvKvjobS1WUSKhGulEehDH093ybv705VfHtRcxRh OdhOEmM98IGYg38+1TsNELMm+WmBG7dFubx81mSQ1/kCuHP57kykHixMHPhecIr8JQxd Hb+o0ASS3QvTupV5CcH40HgpIHOkOymIE9dbRZgvtVhUGnmnfGnknPS33+T2i75LXOo2 Dio0DJI37m+lFPYxD+/fDHy26ecLm4DQFlvtqX1/Zig+JHzPF7Aub6eq2w9rlIG8YCsY 2EQg== X-Gm-Message-State: AOAM53319+Kp24qSSEsvFsEQzt884vpfqusonVW3Xnw90C4GhF/3hjcp s+LT9p4iMqz7pPY8a5JxdyO5heYYSa0+GbCa0DA= X-Google-Smtp-Source: ABdhPJx8jABjwAPkflTLf0tDIwZ9sINLWuWL8r58ETzplbzQ9vN/bx5VRU9o3EskW5h/egE6pcejWF6s01y8esyCx/w= X-Received: by 2002:aa7:c7c2:: with SMTP id o2mr37101563eds.166.1632258168622; Tue, 21 Sep 2021 14:02:48 -0700 (PDT) MIME-Version: 1.0 References: <1631542151-62895-1-git-send-email-tudor.cornea@gmail.com> <1631553801-75072-1-git-send-email-tudor.cornea@gmail.com> <00f7ddfd-33e5-51c2-e881-0546adbf588f@intel.com> In-Reply-To: <00f7ddfd-33e5-51c2-e881-0546adbf588f@intel.com> From: Tudor Cornea Date: Wed, 22 Sep 2021 00:02:37 +0300 Message-ID: To: Ferruh Yigit Cc: Stephen Hemminger , linville@tuxdriver.com, Thomas Monjalon , Mihai Pogonaru , dev@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [dpdk-dev] [PATCH v2] net/af_packet: remove timestamp from packet status X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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 > > Signed-off-by: Tudor Cornea > > > > --- > > 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 */ > > > >