Re: [PATCH net-next 0/4] net/mlx5: Memory optimizations

From: Jiri Pirko <jiri@resnulli.us>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Saeed Mahameed <saeedm@nvidia.com>, Shay Drory <shayd@nvidia.com>,
	Jiri Pirko <jiri@nvidia.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 0/4] net/mlx5: Memory optimizations
Date: Mon, 6 Dec 2021 09:18:37 +0100	[thread overview]
Message-ID: <Ya3HXZ04TtkrHud2@nanopsycho> (raw)
In-Reply-To: <20211202172803.10cd5deb@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

Fri, Dec 03, 2021 at 02:28:03AM CET, kuba@kernel.org wrote:
>On Thu, 2 Dec 2021 18:55:37 +0000 Saeed Mahameed wrote:
>> On Thu, 2021-12-02 at 09:31 -0800, Jakub Kicinski wrote:
>> > On Wed, 1 Dec 2021 10:22:17 +0200 Shay Drory wrote:  
>> > > EQ resides in the host memory. It is RO for host driver, RW by
>> > > device.
>> > > When interrupt is generated EQ entry is placed by device and read
>> > > by driver.
>> > > It indicates about what event occurred such as CQE, async and more.  
>> > 
>> > I understand that. My point was the resource which is being consumed
>> > here is _host_ memory. Is there precedent for configuring host memory
>> > consumption via devlink resource?
>> 
>> it's a device resource size nonetheless, devlink resource API makes
>> total sense.
>
>I disagree. Devlink resources were originally written to partition
>finite device resources. You're just sizing a queue here.
>
>> > I'd even question whether this belongs in devlink in the first place.
>> > It is not global device config in any way. If devlink represents the
>> > entire device it's rather strange to have a case where main instance
>> > limits a size of some resource by VFs and other endpoints can still
>> > choose whatever they want.
>> 
>> This resource is per function instance, we have devlink instance per
>> function, e.g. in the VM, there is a VF devlink instance the VM user
>> can use to control own VF resources. in the PF/Hypervisor, the only
>> devlink representation of the VF will be devlink port function (used
>> for other purposes)
>> 
>> for example:
>> 
>> A tenant can fine-tune a resource size tailored to their needs via the
>> VF's own devlink instance.
>
>Yeah, because it's a device resource. Tenant can consume their host
>DRAM in any way they find suitable.
>
>> An admin can only control or restrict a max size of a resource for a
>> given port function ( the devlink instance that represents the VF in
>> the hypervisor). (note: this patchset is not about that)
>> 
>> > > So far no feedback by other vendors.
>> > > The resources are implemented in generic way, if other vendors
>> > > would
>> > > like to implement them.  
>> > 
>> > Well, I was hoping you'd look around, but maybe that's too much to
>> > ask of a vendor.  
>> 
>> We looked, eq is a common object among many other drivers.
>> and DEVLINK_PARAM_GENERIC_ID_MAX_MACS is already a devlink generic
>> param, and i am sure other vendors have limited macs per VF :) .. 
>> so this applies to all vendors even if they don't advertise it.
>
>Yeah, if you're not willing to model the Event Queue as a queue using
>params seems like a better idea than abusing resources.

I think you are right. On second thought, param look like a better fit.