From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-x636.google.com (mail-pl1-x636.google.com [IPv6:2607:f8b0:4864:20::636]) by mail.toke.dk (Postfix) with ESMTPS id E81C0A58BE0 for ; Sun, 3 Mar 2024 03:00:08 +0100 (CET) Authentication-Results: mail.toke.dk; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20230601 header.b=Y0XLDvil Received: by mail-pl1-x636.google.com with SMTP id d9443c01a7336-1dc3b4b9b62so29742805ad.1 for ; Sat, 02 Mar 2024 18:00:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1709431144; x=1710035944; darn=xdp-project.net; h=content-transfer-encoding:mime-version:subject:references :in-reply-to:message-id:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=WrThd3AbGp2L2TbdibjncZIHTv9jShOpMlQ84CCLklk=; b=Y0XLDviljCZY6ZEdC0kPCYS2rajB4gHV3JKkFirFhSAuNEpq1No793mAsYDmSSn+WX mCZpYGvikPFP7iKgt8FhNwpXrojWLJm6mEgaVOaJbLO2Z9H1fhvux8/FESSobL32oGS4 6khN884JmO2AZI6xBH2TWWfeXmwyzRjXQqvhRaZVBFHBvsGZi62AJ50Vm/KO7qKOhRye G1Sso0Rw3ya5ROqcekvObm2KC2GR7w9bKrggrLNRUN1pk+9HSzb+1r9zrx1r8op85HC3 +nQ+CCB0VnX1piPpcDNg8MKvTrVRo77Sa4zVDZE/NXvsVwBvIgovjWZ9f+Iy0/GhZebJ /37g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709431144; x=1710035944; h=content-transfer-encoding:mime-version:subject:references :in-reply-to:message-id:cc:to:from:date:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=WrThd3AbGp2L2TbdibjncZIHTv9jShOpMlQ84CCLklk=; b=CYONcKT2tHYQrc10mK1lNcJn+3qTSZ8+O/CL1cOVF1MbJZELGrXbj4Z2wktl88JGg1 SpEgzjux08BvnQ3PM1qVL0VRRS+MoNaAJWwPxNCiR7ayL/l2CtJdRD1nrD9miUuVG8aB HNDqoSMXgcB52vr66jo08zJv5I4WmphJuikTaTSiB8WSyIHqBClnqsighfwdmLPD7spM S/Wu9zk5/r+RXDlvEOlzJHSa7sAfWU4+bvSLEAplQGdhSZ/YYA6zEtnt+q8XXOxEAPov G1GziUygZoC9OY8It9sZgQkYiIySP6gAKqDBg/tW0O+A7JarF05923PoYka48KrUmxSy C/mg== X-Forwarded-Encrypted: i=1; AJvYcCUVUwU11lqQlAmUozkpYw8gxxaxboEh3HXnKSiGCQOWD9siUXYaQHJcXHj+YAMeKsiiy4rxa7OWe6txrZk6ZxJe2a0vSsxTuJt3 X-Gm-Message-State: AOJu0YxwfpkgMGQeQfP7j58ldpCvv3xJ6plwCUyFqcgnDHYJv/tj8Xsx 8+B42qn3An+uSQWJo3MMnuUJZvOb1jjo9XQRbnIOlxl6nFXJErFI X-Google-Smtp-Source: AGHT+IEpcJ1ppLdYI3v6PLpHlNkGd8s29BlMzJkoxyiUaoL1N4wuBSlJRKARDn+uAIHWuZ2lCr5vRA== X-Received: by 2002:a17:902:a513:b0:1dc:b77e:1978 with SMTP id s19-20020a170902a51300b001dcb77e1978mr10346412plq.28.1709431144407; Sat, 02 Mar 2024 17:59:04 -0800 (PST) Received: from localhost (071-084-162-223.res.spectrum.com. [71.84.162.223]) by smtp.gmail.com with ESMTPSA id l5-20020a170903120500b001dc3ef7aa2bsm5827341plh.49.2024.03.02.17.59.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 02 Mar 2024 17:59:03 -0800 (PST) Date: Sat, 02 Mar 2024 17:58:59 -0800 From: John Fastabend To: "Song, Yoong Siang" , John Fastabend , "Brandeburg, Jesse" , "Nguyen, Anthony L" , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Richard Cochran , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , Stanislav Fomichev , "Gomes, Vinicius" , "Bezdeka, Florian" , Andrii Nakryiko , Eduard Zingerman , Mykola Lysenko , Martin KaFai Lau , Song Liu , Yonghong Song , KP Singh , Hao Luo , Jiri Olsa , Shuah Khan Message-ID: <65e3d963c6dc0_8ee3b20879@john.notmuch> In-Reply-To: References: <20240301162348.898619-1-yoong.siang.song@intel.com> <20240301162348.898619-3-yoong.siang.song@intel.com> <65e2165a89ed0_5dcfe20823@john.notmuch> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Message-ID-Hash: TKCYOCTVF2DCBHPTRLEXMR4L376OYN7I X-Message-ID-Hash: TKCYOCTVF2DCBHPTRLEXMR4L376OYN7I X-MailFrom: john.fastabend@gmail.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: "intel-wired-lan@lists.osuosl.org" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "bpf@vger.kernel.org" , "linux-kselftest@vger.kernel.org" , "xdp-hints@xdp-project.net" X-Mailman-Version: 3.3.9 Precedence: list Subject: [xdp-hints] Re: [PATCH iwl-next,v2 2/2] igc: Add Tx hardware timestamp request for AF_XDP zero-copy packet List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Song, Yoong Siang wrote: > On Saturday, March 2, 2024 1:55 AM, John Fastabend wrote: > >Song Yoong Siang wrote: > >> This patch adds support to per-packet Tx hardware timestamp request to > >> AF_XDP zero-copy packet via XDP Tx metadata framework. Please note that > >> user needs to enable Tx HW timestamp capability via igc_ioctl() with > >> SIOCSHWTSTAMP cmd before sending xsk Tx hardware timestamp request. > >> > >> Same as implementation in RX timestamp XDP hints kfunc metadata, Timer 0 > >> (adjustable clock) is used in xsk Tx hardware timestamp. i225/i226 have > >> four sets of timestamping registers. Both *skb and *xsk_tx_buffer pointers > >> are used to indicate whether the timestamping register is already occupied. > >> > >> Furthermore, a boolean variable named xsk_pending_ts is used to hold the > >> transmit completion until the tx hardware timestamp is ready. This is > >> because, for i225/i226, the timestamp notification event comes some time > >> after the transmit completion event. The driver will retrigger hardware irq > >> to clean the packet after retrieve the tx hardware timestamp. > >> > >> Besides, xsk_meta is added into struct igc_tx_timestamp_request as a hook > >> to the metadata location of the transmit packet. When the Tx timestamp > >> interrupt is fired, the interrupt handler will copy the value of Tx hwts > >> into metadata location via xsk_tx_metadata_complete(). > >> > >> Co-developed-by: Lai Peter Jun Ann > >> Signed-off-by: Lai Peter Jun Ann > >> Signed-off-by: Song Yoong Siang > >> --- > > > >[...] > > > >> > >> +static void igc_xsk_request_timestamp(void *_priv) > >> +{ > >> + struct igc_metadata_request *meta_req = _priv; > >> + struct igc_ring *tx_ring = meta_req->tx_ring; > >> + struct igc_tx_timestamp_request *tstamp; > >> + u32 tx_flags = IGC_TX_FLAGS_TSTAMP; > >> + struct igc_adapter *adapter; > >> + unsigned long lock_flags; > >> + bool found = false; > >> + int i; > >> + > >> + if (test_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags)) { > >> + adapter = netdev_priv(tx_ring->netdev); > >> + > >> + spin_lock_irqsave(&adapter->ptp_tx_lock, lock_flags); > >> + > >> + /* Search for available tstamp regs */ > >> + for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) { > >> + tstamp = &adapter->tx_tstamp[i]; > >> + > >> + if (tstamp->skb) > >> + continue; > >> + > >> + found = true; > >> + break; > > > >Not how I would have written this loop construct seems a bit odd > >to default break but it works. > > Hi John, > First of all, thank you for reviewing the patch. > I agree that we can make the loop better. > How about I change the loop to below: That is more natural to me, but whatever reads best for you is probably ok. > > for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) { > tstamp = &adapter->tx_tstamp[i]; > > if (!tstamp->skb) { > found = true; > break; > } > } > > > > >> + } > >> + > >> + /* Return if no available tstamp regs */ > >> + if (!found) { > >> + adapter->tx_hwtstamp_skipped++; > >> + spin_unlock_irqrestore(&adapter->ptp_tx_lock, > >> + lock_flags); > >> + return; > >> + } > > > >[...] > > > >> > >> +static void igc_ptp_free_tx_buffer(struct igc_adapter *adapter, > >> + struct igc_tx_timestamp_request *tstamp) > >> +{ > >> + if (tstamp->buffer_type == IGC_TX_BUFFER_TYPE_XSK) { > >> + /* Release the transmit completion */ > >> + tstamp->xsk_tx_buffer->xsk_pending_ts = false; > >> + tstamp->xsk_tx_buffer = NULL; > >> + tstamp->buffer_type = 0; > >> + > >> + /* Trigger txrx interrupt for transmit completion */ > >> + igc_xsk_wakeup(adapter->netdev, tstamp->xsk_queue_index, 0); > > > >Just curious because I didn't find it. Fairly sure I just need to look more, > >but don't you want to still 'tstamp->skb = NULL' in this path somewhere? > >It looks like triggering the tx interrupt again with buffer_type == 0 wouldn't > >do the null. > > > >I suspect I just missed it. > > Since the timestamp register will only be used by either skb or xsk, > So we make tstamp->xsk_tx_buffer and tstamp->skb as union, > Then based on tstamp->buffer_type to decide whether > tstamp->xsk_tx_buffer or tstamp->skb should be used. > > My thought is, by setting tstamp->xsk_tx_buffer=NULL, > tstamp->skb will become NULL as well, and vice versa. Seems good to me. Maybe a comment though? Otherwise I suspect next person to read the code will have to spend the extra time to track it down as well. > > Thanks & Regards > Siang Also feel free to carry my ack into the v2 if you make the couple small nitpick changes. Acked-by: John Fastabend