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 smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (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 8D8FDC87FCA for ; Sun, 10 Aug 2025 11:00:36 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 412DB60F27; Sun, 10 Aug 2025 11:00:36 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id 0z8e1wC2NyKl; Sun, 10 Aug 2025 11:00:35 +0000 (UTC) X-Comment: SPF check N/A for local connections - client-ip=140.211.166.142; helo=lists1.osuosl.org; envelope-from=intel-wired-lan-bounces@osuosl.org; receiver= DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org B4BE160F3A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=osuosl.org; s=default; t=1754823635; bh=y1OD/fs97lJSnzwe5rHz5qP8Wlog8IOWL6QwAL84Y1g=; h=Date:To:Cc:References:From:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=pnA5ZS+ec1Fr40EUaV3X/1NYQpKukgmJA0ouPtdMILviwtjDo5x33kJNvsgvopHkT bNcSG5WT2igpFiupfmZ5eyZQRN9WLGNhGDRng0MbcHmfXYlbtFoCwQgTgff4DMVxpZ 9sNs1uhezI1+W4dC+WlE3cOX3XHZ1z3xkl5WS4sSr323AQoHrNPHdd62f3zUEkDesb 05FlMi7EvtJ7Wfi84677j160545v+Klu5TmkOlz7fM4gupwrpoiRDNpFKgMpQdTr2D vKJoCuusO9Ox4WErbE4MPO3eyiJmdPDy2pCaMu5iIzcz765dBaHLyS0NJgTPMvOHM6 Oz1FtYFFjuI1A== Received: from lists1.osuosl.org (lists1.osuosl.org [140.211.166.142]) by smtp3.osuosl.org (Postfix) with ESMTP id B4BE160F3A; Sun, 10 Aug 2025 11:00:35 +0000 (UTC) Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists1.osuosl.org (Postfix) with ESMTP id 207A3138 for ; Sun, 10 Aug 2025 11:00:34 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 1256440DA0 for ; Sun, 10 Aug 2025 11:00:34 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id CPCxwASzwGe9 for ; Sun, 10 Aug 2025 11:00:33 +0000 (UTC) X-Greylist: delayed 398 seconds by postgrey-1.37 at util1.osuosl.org; Sun, 10 Aug 2025 11:00:32 UTC DMARC-Filter: OpenDMARC Filter v1.4.2 smtp4.osuosl.org 3EC2C40CD7 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 3EC2C40CD7 Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=95.215.58.171; helo=out-171.mta1.migadu.com; envelope-from=vadim.fedorenko@linux.dev; receiver= Received: from out-171.mta1.migadu.com (out-171.mta1.migadu.com [95.215.58.171]) by smtp4.osuosl.org (Postfix) with ESMTPS id 3EC2C40CD7 for ; Sun, 10 Aug 2025 11:00:32 +0000 (UTC) Message-ID: Date: Sun, 10 Aug 2025 11:52:55 +0100 MIME-Version: 1.0 To: Jakub Kicinski Cc: Andrew Lunn , Michael Chan , Pavan Chebbi , Tariq Toukan , Gal Pressman , intel-wired-lan@lists.osuosl.org, Donald Hunter , Carolina Jubran , Paolo Abeni , Simon Horman , netdev@vger.kernel.org References: <20250807155924.2272507-1-vadfed@meta.com> <20250808131522.0dc26de4@kernel.org> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Vadim Fedorenko In-Reply-To: <20250808131522.0dc26de4@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1754823226; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y1OD/fs97lJSnzwe5rHz5qP8Wlog8IOWL6QwAL84Y1g=; b=QQQ37w/YI/SfcG3Zp/Hf1ZO5W1fzmDFWZUzGSlKSN1aQWiwobf+1TkHMJAEjVJ0QaqXkBT m0z6rGFWi2n+n0G1kQRo4lypAMVR5X9oCtSJlQ1Qbj7wIPik3NhH3I60o8yGPwOZ39/VPl +sZ0KwK/wGR5UificM2Z9siE6qpIfJg= X-Mailman-Original-Authentication-Results: smtp4.osuosl.org; dmarc=pass (p=none dis=none) header.from=linux.dev X-Mailman-Original-Authentication-Results: smtp4.osuosl.org; dkim=pass (1024-bit key, unprotected) header.d=linux.dev header.i=@linux.dev header.a=rsa-sha256 header.s=key1 header.b=QQQ37w/Y Subject: Re: [Intel-wired-lan] [RFC PATCH v4] ethtool: add FEC bins histogramm report X-BeenThere: intel-wired-lan@osuosl.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Intel Wired Ethernet Linux Kernel Driver Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-wired-lan-bounces@osuosl.org Sender: "Intel-wired-lan" On 08/08/2025 21:15, Jakub Kicinski wrote: > On Thu, 7 Aug 2025 08:59:24 -0700 Vadim Fedorenko wrote: >> + /* add FEC bins information */ >> + len += (nla_total_size(0) + /* _A_FEC_HIST */ >> + nla_total_size(4) + /* _A_FEC_HIST_BIN_LOW */ >> + nla_total_size(4) + /* _A_FEC_HIST_BIN_HI */ >> + /* _A_FEC_HIST_BIN_VAL + per-lane values */ >> + nla_total_size_64bit(sizeof(u64) * >> + (1 + ETHTOOL_MAX_LANES))) * > > That's the calculation for basic stats because they are reported as a > raw array. Each nla_put() should correspond to a nla_total_size(). > This patch nla_put()s things individually. > >> + ETHTOOL_FEC_HIST_MAX; >> + } >> >> return len; >> } >> >> +static int fec_put_hist(struct sk_buff *skb, const struct ethtool_fec_hist *hist) >> +{ >> + const struct ethtool_fec_hist_range *ranges = hist->ranges; >> + const struct ethtool_fec_hist_value *values = hist->values; >> + struct nlattr *nest; >> + int i, j; >> + >> + if (!ranges) >> + return 0; >> + >> + for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) { >> + if (i && !ranges[i].low && !ranges[i].high) >> + break; >> + >> + if (WARN_ON_ONCE(values[i].bin_value == ETHTOOL_STAT_NOT_SET)) >> + break; >> + >> + nest = nla_nest_start(skb, ETHTOOL_A_FEC_STAT_HIST); >> + if (!nest) >> + return -EMSGSIZE; >> + >> + if (nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_LOW, >> + ranges[i].low) || >> + nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_HIGH, >> + ranges[i].high) || >> + nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL, >> + values[i].bin_value)) >> + goto err_cancel_hist; >> + for (j = 0; j < ETHTOOL_MAX_LANES; j++) { >> + if (values[i].bin_value_per_lane[j] == ETHTOOL_STAT_NOT_SET) >> + break; >> + nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE, >> + values[i].bin_value_per_lane[j]); > > TBH I'm a bit unsure if this is really worth breaking out into > individual nla_puts(). We generally recommend that, but here it's > an array of simple ints.. maybe we're better of with a binary / C > array of u64. Like the existing FEC stats but without also folding > the total value into index 0. Well, the current implementation is straight forward. Do you propose to have drivers fill in the amount of lanes they have histogram for, or should we always put array of ETHTOOL_MAX_LANES values and let user-space to figure out what to show? From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-189.mta1.migadu.com (out-189.mta1.migadu.com [95.215.58.189]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5B1FE2E36F1 for ; Sun, 10 Aug 2025 10:53:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.189 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754823231; cv=none; b=WF8dYB0bRgDDk/oCrTnsPovCZqYlVH9SyaBz2bJkGsyty7mFzeBJyBo/912fb9zSVg+PCN11T/eSnPxitY8mkFaH7LpPP/Mb5kmloo3R9sLEFPk6j7diVR9WSycr0gv4y0hklXikoOA7okg1M4r2G3A3XM7fUeJrRAZ4kfueZEI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754823231; c=relaxed/simple; bh=sqHeBdCK1S81WhduKgiYc1nbZR02E0yF7GBV6EoVe/I=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=o+rYExeQYwU49A86GJQOGT0X6EUpNen/XiT/SFhyoBcv/IkFHZK+t9ko8oufhbUTWQNL5rlOs1MT/U1bVkhRu7qqwUzj/RGcBPhEqmEZ47KN5ZnEOxIH0Q7Mc/H3in+BV1iRZLPYdhYxAJQTDmkSxW4+Yo6MLYgevMZS9EAo8zY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=QQQ37w/Y; arc=none smtp.client-ip=95.215.58.189 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="QQQ37w/Y" Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1754823226; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y1OD/fs97lJSnzwe5rHz5qP8Wlog8IOWL6QwAL84Y1g=; b=QQQ37w/YI/SfcG3Zp/Hf1ZO5W1fzmDFWZUzGSlKSN1aQWiwobf+1TkHMJAEjVJ0QaqXkBT m0z6rGFWi2n+n0G1kQRo4lypAMVR5X9oCtSJlQ1Qbj7wIPik3NhH3I60o8yGPwOZ39/VPl +sZ0KwK/wGR5UificM2Z9siE6qpIfJg= Date: Sun, 10 Aug 2025 11:52:55 +0100 Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [RFC PATCH v4] ethtool: add FEC bins histogramm report To: Jakub Kicinski Cc: Andrew Lunn , Michael Chan , Pavan Chebbi , Tariq Toukan , Gal Pressman , intel-wired-lan@lists.osuosl.org, Donald Hunter , Carolina Jubran , Paolo Abeni , Simon Horman , netdev@vger.kernel.org References: <20250807155924.2272507-1-vadfed@meta.com> <20250808131522.0dc26de4@kernel.org> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Vadim Fedorenko In-Reply-To: <20250808131522.0dc26de4@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT On 08/08/2025 21:15, Jakub Kicinski wrote: > On Thu, 7 Aug 2025 08:59:24 -0700 Vadim Fedorenko wrote: >> + /* add FEC bins information */ >> + len += (nla_total_size(0) + /* _A_FEC_HIST */ >> + nla_total_size(4) + /* _A_FEC_HIST_BIN_LOW */ >> + nla_total_size(4) + /* _A_FEC_HIST_BIN_HI */ >> + /* _A_FEC_HIST_BIN_VAL + per-lane values */ >> + nla_total_size_64bit(sizeof(u64) * >> + (1 + ETHTOOL_MAX_LANES))) * > > That's the calculation for basic stats because they are reported as a > raw array. Each nla_put() should correspond to a nla_total_size(). > This patch nla_put()s things individually. > >> + ETHTOOL_FEC_HIST_MAX; >> + } >> >> return len; >> } >> >> +static int fec_put_hist(struct sk_buff *skb, const struct ethtool_fec_hist *hist) >> +{ >> + const struct ethtool_fec_hist_range *ranges = hist->ranges; >> + const struct ethtool_fec_hist_value *values = hist->values; >> + struct nlattr *nest; >> + int i, j; >> + >> + if (!ranges) >> + return 0; >> + >> + for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) { >> + if (i && !ranges[i].low && !ranges[i].high) >> + break; >> + >> + if (WARN_ON_ONCE(values[i].bin_value == ETHTOOL_STAT_NOT_SET)) >> + break; >> + >> + nest = nla_nest_start(skb, ETHTOOL_A_FEC_STAT_HIST); >> + if (!nest) >> + return -EMSGSIZE; >> + >> + if (nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_LOW, >> + ranges[i].low) || >> + nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_HIGH, >> + ranges[i].high) || >> + nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL, >> + values[i].bin_value)) >> + goto err_cancel_hist; >> + for (j = 0; j < ETHTOOL_MAX_LANES; j++) { >> + if (values[i].bin_value_per_lane[j] == ETHTOOL_STAT_NOT_SET) >> + break; >> + nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE, >> + values[i].bin_value_per_lane[j]); > > TBH I'm a bit unsure if this is really worth breaking out into > individual nla_puts(). We generally recommend that, but here it's > an array of simple ints.. maybe we're better of with a binary / C > array of u64. Like the existing FEC stats but without also folding > the total value into index 0. Well, the current implementation is straight forward. Do you propose to have drivers fill in the amount of lanes they have histogram for, or should we always put array of ETHTOOL_MAX_LANES values and let user-space to figure out what to show?