Received-SPF: pass (google.com: domain of linux-kernel+bounces-85411-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <84e4f0d70c5552dd7fa350c61c28de9637628ee6.camel@sipsolutions.net>
Subject: Re: [PATCH v2 2/4] devcoredump: Add dev_coredumpm_timeout()
From: Johannes Berg <johannes@sipsolutions.net>
To: =?ISO-8859-1?Q?Jos=E9?= Roberto de Souza <jose.souza@intel.com>, 
	linux-kernel@vger.kernel.org, intel-xe@lists.freedesktop.org
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>, Mukesh Ojha
 <quic_mojha@quicinc.com>,  Jonathan Cavitt <jonathan.cavitt@intel.com>
Date: Wed, 28 Feb 2024 18:05:51 +0100
In-Reply-To: <20240228165709.82089-2-jose.souza@intel.com>
References: <20240228165709.82089-1-jose.souza@intel.com>
	 <20240228165709.82089-2-jose.souza@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

> Current 5-minute timeout may be too short for users to search and
> understand what needs to be done to capture coredump to report bugs.

Conceptually, I'm not sure I understand this. Users should probably have
a script to capture coredumps to a file in the filesystem, possibly with
additional data such as 'dmesg' at the time of the dump.

Having this stick around longer in core kernel memory (not even
swappable) seems like a bad idea?

What kind of timeout were you thinking? Maybe you'd want 10 minutes? An
hour?

Also, then, why should the timeout be device-specific? If the user is
going to need time to find stuff, then surely that applies regardless of
the device?

So ... I guess I don't really like this, and don't really see how it
makes sense. Arguably, 5 minutes even is too long, not too short,
because you should have scripting that captures it, writes it to disk,
and all that can happen in the space of seconds, rather than minutes.
It's trivial to write such a script with a udev trigger or similar.

If we wanted to, we could even have a script that not only captures it
to disk, but also deletes it again from disk after a day or something,
so if you didn't care you don't get things accumulating. But I don't see
why the kernel should need to hang on to all the (possibly big) core
dump in RAM, for whatever time. And I also don't like the device-
dependency very much, TBH.


But if we do go there eventually:

> +void dev_coredumpm(struct device *dev, struct module *owner,
> +		   void *data, size_t datalen, gfp_t gfp,
> +		   ssize_t (*read)(char *buffer, loff_t offset, size_t count,
> +				   void *data, size_t datalen),
> +		   void (*free)(void *data))
> +{
> +	dev_coredumpm_timeout(dev, owner, data, datalen, gfp, read, free,
> +			      DEVCD_TIMEOUT);
> +}
>  EXPORT_SYMBOL_GPL(dev_coredumpm);

This could be a trivial static inline now, if you just put DEVCD_TIMEOUT
into the header file. Seems better than exporting another whole function
for it. Then you also don't need the no-op version of it.

johannes