Received-SPF: pass (google.com: domain of linux-kernel+bounces-179770-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 15 May 2024 11:46:26 +0100
From: Simon Horman <horms@kernel.org>
To: Ronak Doshi <ronak.doshi@broadcom.com>
Cc: netdev@vger.kernel.org,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 2/4] vmxnet3: add latency measurement support in
 vmxnet3
Message-ID: <20240515104626.GE154012@kernel.org>
References: <20240514182050.20931-1-ronak.doshi@broadcom.com>
 <20240514182050.20931-3-ronak.doshi@broadcom.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240514182050.20931-3-ronak.doshi@broadcom.com>

On Tue, May 14, 2024 at 11:20:47AM -0700, Ronak Doshi wrote:
> This patch enhances vmxnet3 to support latency measurement.
> This support will help to track the latency in packet processing
> between guest virtual nic driver and host. For this purpose, we
> introduce a new timestamp ring in vmxnet3 which will be per Tx/Rx
> queue. This ring will be used to carry timestamp of the packets
> which will be used to calculate the latency.
> 
> Signed-off-by: Ronak Doshi <ronak.doshi@broadcom.com>
> Acked-by: Guolin Yang <guolin.yang@broadcom.com>

..

> index b3f3136cc8be..74cb63e3d311 100644
> --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> @@ -143,6 +143,29 @@ vmxnet3_tq_stop(struct vmxnet3_tx_queue *tq, struct vmxnet3_adapter *adapter)
>  	netif_stop_subqueue(adapter->netdev, (tq - adapter->tx_queue));
>  }
>  
> +static u64
> +vmxnet3_get_cycles(int pmc)
> +{
> +	u32 low, high;
> +
> +	asm volatile("rdpmc" : "=a" (low), "=d" (high) : "c" (pmc));
> +	return (low | ((u_int64_t)high << 32));
> +}

Hi Ronak,

This seems to open-code the rdpmc macro.

And it also seems to exclude compilation of this driver other than for x86.
This seems undesirable as, in general, networking drivers are supposed to
be architecture independent. I'd say, doubly so, for software devices.

Moreover, rdpmc outside of x86 architecture-specific code seems highly
unusual to me. So I wonder if there is a better approach to the problem at
hand.

If not, I would suggest making this feature optional and only compiled
for x86. That might mean factoring it out into a different file. I'm
unsure.

If not, I think the driver's Kconfig needs to be updated to reflect
that it can only be compiled for x86.

..