From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1EFD4C54798 for ; Sun, 3 Mar 2024 01:59:09 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 9FF54827CA; Sun, 3 Mar 2024 01:59:08 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id a6hZZY0R1P_9; Sun, 3 Mar 2024 01:59:07 +0000 (UTC) X-Comment: SPF check N/A for local connections - client-ip=140.211.166.34; helo=ash.osuosl.org; envelope-from=intel-wired-lan-bounces@osuosl.org; receiver= DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org D5BAF82883 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osuosl.org; s=default; t=1709431147; bh=WrThd3AbGp2L2TbdibjncZIHTv9jShOpMlQ84CCLklk=; h=Date:From:To:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: Cc:From; b=ziYl1eR3FmWiYjUfzn5ngyf8Y0ToKZIU1vpZhzXvhJhwgZfLFT0owi8wBN87coiMD lp7lPvkt3hLBaXnU+Poz5ZgvOns9U6xP0Bv3ehz9Kat3Uxn1iK/CApEEgTtvugH/RV bhbO1zP3C4IOAHHsaOGua/N3e1Ch0ig2v8VS1OSv1XAO8c3L6DN5e0syuBeWH77x13 llIUXW1F0qlHi0zg2biONiR3Z/C9I6r68mB/oE1K6fSvA5FvhPrDW+0AJuoU/XyG1F CxAPemw11sxPz6pa7OCOfYdJHVe9EwxTsUBE0iWMaHtnvgS2sVeO75VYlXWcvd3QP8 Trux6mh8ZRArg== Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by smtp1.osuosl.org (Postfix) with ESMTP id D5BAF82883; Sun, 3 Mar 2024 01:59:07 +0000 (UTC) Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by ash.osuosl.org (Postfix) with ESMTP id 412D41BF20F for ; Sun, 3 Mar 2024 01:59:06 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 36BB241554 for ; Sun, 3 Mar 2024 01:59:06 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id b_52vDDAYCnw for ; Sun, 3 Mar 2024 01:59:05 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=2607:f8b0:4864:20::62b; helo=mail-pl1-x62b.google.com; envelope-from=john.fastabend@gmail.com; receiver= DMARC-Filter: OpenDMARC Filter v1.4.2 smtp4.osuosl.org 5D743414CD DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 5D743414CD Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by smtp4.osuosl.org (Postfix) with ESMTPS id 5D743414CD for ; Sun, 3 Mar 2024 01:59:05 +0000 (UTC) Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1dc3b4b9b62so29742775ad.1 for ; Sat, 02 Mar 2024 17:59:05 -0800 (PST) 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=wScibtLAG9c1hiMiiIQ/ZSFwgIbN/wUTb/S+En+NpJSnZRnNmusKICHwiZUAl/2FS+ vXldDUPY0yq2F2fSaGF6TIcCFZKWLoHLM79N9I5VR3Q8W8NhtCAblMZfXrne6Akumtz0 qcBQ2NTWNszBKjiXdAeiOJ0ZkfvNYwdDzL9JTMGxNtxC2bkBaXTPpdX4NTbbGtK+dLIN 6HXhDGrRQuwS2yhJrf82CEGiZfwh5JlobRBF0+nUXZevddCA3HAyPr0+qhpzdqLeVWle xqn99dfOQ64uIhN0lnE6a5ETbneOiydh/v6J5kp2nJKI97xPJGOD/pWtXVWsoc6/Hnl/ PA9w== X-Gm-Message-State: AOJu0YxCdQBUzctEOaZ1X8H2P4ubB3OsBRjn5FfAkG7bF5fMhjvF8ceh DDA3OQU8+K0GxK6UHzlQpD9qGzroQxwSww2P2fEwlmA+8q/g5u5c 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 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1709431144; x=1710035944; darn=lists.osuosl.org; 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=mFiagDTPcU1c1otZ6rGgVez/SwXeXt1P3Dnj8NDMUj6OiFbzjk3YfdvzYlmUrAq07j y8ebc/yLnH4FCR4LaVSAv4pM91+JWFNxvI4fn8I4uFJTTxgqmbL8lKWyGaluW5n2da/1 zm/ERmrOEszKyEtCSMV4bxioCHXgFQmdplwG32debjD1ZkuCgD39YqIC6VjHHP8v2Igs fAIbgZzI/phxPnNUyK5aDzswL1DEWzl3TO2AQNXGsbVpMrcifP0AH49e2fHUNnbzcLJ9 aWld6uaThqqQLYiu/Nsl5RTftwuGCG/OCKBZi8ddvjU0tcETcZzEMDBP+QBV08EkUxxX GtOw== X-Mailman-Original-Authentication-Results: smtp4.osuosl.org; dmarc=pass (p=none dis=none) header.from=gmail.com X-Mailman-Original-Authentication-Results: smtp4.osuosl.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20230601 header.b=mFiagDTP Subject: Re: [Intel-wired-lan] [PATCH iwl-next, v2 2/2] igc: Add Tx hardware timestamp request for AF_XDP zero-copy packet X-BeenThere: intel-wired-lan@osuosl.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Wired Ethernet Linux Kernel Driver Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "xdp-hints@xdp-project.net" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "intel-wired-lan@lists.osuosl.org" , "linux-kselftest@vger.kernel.org" , "bpf@vger.kernel.org" Errors-To: intel-wired-lan-bounces@osuosl.org Sender: "Intel-wired-lan" 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