Re: [PATCH net-next 0/9] devlink: Add support for region access

From: Andrew Lunn <andrew@lunn.ch>
To: Alex Vesker <valex@mellanox.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org, Tariq Toukan <tariqt@mellanox.com>,
	Jiri Pirko <jiri@mellanox.com>
Subject: Re: [PATCH net-next 0/9] devlink: Add support for region access
Date: Fri, 30 Mar 2018 16:34:03 +0200	[thread overview]
Message-ID: <20180330143403.GD28244@lunn.ch> (raw)
In-Reply-To: <28b99a08-1967-3044-4010-0faa5d6bfc14@mellanox.com>

> >And it seems to want contiguous pages. How well does that work after
> >the system has been running for a while and memory is fragmented?
> 
> The allocation can be changed, there is no read need for contiguous pages.
> It is important to note that we the amount of snapshots is limited by the
> driver
> this can be based on the dump size or expected frequency of collection.
> I also prefer not to pre-allocate this memory.

The driver code also asks for a 1MB contiguous chunk of memory!  You
really should think about this API, how can you avoid double memory
allocations. And can kvmalloc be used. But then you get into the
problem for DMA'ing the memory from the device...

This API also does not scale. 1MB is actually quite small. I'm sure
there is firmware running on CPUs with a lot more than 1MB of RAM.
How well does with API work with 64MB? Say i wanted to snapshot my
GPU? Or the MC/BMC?

> Dump in devlink means provide all the data, saying dump address x length y
> sounds
> confusing.  Do you see this as a critical issue?

No, i don't. But nearly every set of tools i've used has one command.
eg uboot, coreboot, gdb, od, hexdump. Even ethtool has [offset N] [length N]

How many tools can you name which have two different command, rather
than one with options?

      Andrew