[v2,2/4] devcoredump: Add dev_coredumpm_timeout()
Commit Message
Add function to set a custom coredump timeout.
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.
v2:
- replace dev_coredump_timeout_set() by dev_coredumpm_timeout() (Mukesh)
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Mukesh Ojha <quic_mojha@quicinc.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/base/devcoredump.c | 44 +++++++++++++++++++++++++++++++------
include/linux/devcoredump.h | 18 +++++++++++++++
2 files changed, 55 insertions(+), 7 deletions(-)
Comments
> 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
On Wed, 2024-02-28 at 18:05 +0100, Johannes Berg wrote:
> > 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.
In my opinion, the timeout should depend on the type of device driver.
In the case of server-class Ethernet cards, where corporate users automate most tasks, five minutes might even be considered excessive.
For our case, GPUs, users might experience minor glitches and only search for what happened after finishing their current task (writing an email,
ending a gaming match, watching a YouTube video, etc.).
If they land on https://drm.pages.freedesktop.org/intel-docs/how-to-file-i915-bugs.html or the future Xe version of that page, following the
instructions alone may take inexperienced Linux users more than five minutes.
I have set the timeout to one hour in the Xe driver, but this could increase if we start receiving user complaints.
>
>
>
> 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.
works for me too, will wait an ack in the above explanation.
>
> johannes
>
On Wed, 2024-02-28 at 17:56 +0000, Souza, Jose wrote:
>
> In my opinion, the timeout should depend on the type of device driver.
>
> In the case of server-class Ethernet cards, where corporate users automate most tasks, five minutes might even be considered excessive.
>
> For our case, GPUs, users might experience minor glitches and only search for what happened after finishing their current task (writing an email,
> ending a gaming match, watching a YouTube video, etc.).
> If they land on https://drm.pages.freedesktop.org/intel-docs/how-to-file-i915-bugs.html or the future Xe version of that page, following the
> instructions alone may take inexperienced Linux users more than five minutes.
That's all not wrong, but I don't see why you wouldn't automate this
even on end user machines? I feel you're boxing the problem in by
wanting to solve it entirely in the kernel?
> I have set the timeout to one hour in the Xe driver, but this could increase if we start receiving user complaints.
At an hour now, people will probably start arguing that "indefinitely"
is about right? But at that point you're probably back to persisting
them on disk anyway? Or maybe glitches happen during logout/shutdown ...
Anyway, I don't want to block this because I just don't care enough
about how you do things, but I think the kernel is the wrong place to
solve this problem... The intent here was to give some userspace time to
grab it (and yes for that 5 minutes is already way too long), not the
users. That's also part of the reason we only hold on to a single
instance, since I didn't want it to keep consuming more and more memory
for it if happens repeatedly.
johannes
On Fri, 2024-03-01 at 09:38 +0100, Johannes Berg wrote:
> > On Wed, 2024-02-28 at 17:56 +0000, Souza, Jose wrote:
> > > >
> > > > In my opinion, the timeout should depend on the type of device driver.
> > > >
> > > > In the case of server-class Ethernet cards, where corporate users automate most tasks, five minutes might even be considered excessive.
> > > >
> > > > For our case, GPUs, users might experience minor glitches and only search for what happened after finishing their current task (writing an email,
> > > > ending a gaming match, watching a YouTube video, etc.).
> > > > If they land on https://drm.pages.freedesktop.org/intel-docs/how-to-file-i915-bugs.html or the future Xe version of that page, following the
> > > > instructions alone may take inexperienced Linux users more than five minutes.
> >
> > That's all not wrong, but I don't see why you wouldn't automate this
> > even on end user machines? I feel you're boxing the problem in by
> > wanting to solve it entirely in the kernel?
The other part of the stack that we provide are the libraries implementing Vulkan and OpenGL APIs, I don't think we could ship scripts that needs
elevated privileges to read and store coredump.
> >
> > > > I have set the timeout to one hour in the Xe driver, but this could increase if we start receiving user complaints.
> >
> > At an hour now, people will probably start arguing that "indefinitely"
> > is about right? But at that point you're probably back to persisting
> > them on disk anyway? Or maybe glitches happen during logout/shutdown ...
i915 driver don't use coredump and it persist the error dump in memory until user frees it or reboot it and we got no complains.
> >
> > Anyway, I don't want to block this because I just don't care enough
> > about how you do things, but I think the kernel is the wrong place to
> > solve this problem... The intent here was to give some userspace time to
> > grab it (and yes for that 5 minutes is already way too long), not the
> > users. That's also part of the reason we only hold on to a single
> > instance, since I didn't want it to keep consuming more and more memory
> > for it if happens repeatedly.
> >
okay so will move forward with other version applying your suggestion to make dev_coredumpm() static inline and move to the header.
thank you for the feedback
> > johannes
@@ -328,7 +328,8 @@ void dev_coredump_put(struct device *dev)
EXPORT_SYMBOL_GPL(dev_coredump_put);
/**
- * dev_coredumpm - create device coredump with read/free methods
+ * dev_coredumpm_timeout - create device coredump with read/free methods with a
+ * custom timeout.
* @dev: the struct device for the crashed device
* @owner: the module that contains the read/free functions, use %THIS_MODULE
* @data: data cookie for the @read/@free functions
@@ -336,17 +337,20 @@ EXPORT_SYMBOL_GPL(dev_coredump_put);
* @gfp: allocation flags
* @read: function to read from the given buffer
* @free: function to free the given buffer
+ * @timeout: time in jiffies to remove coredump
*
* Creates a new device coredump for the given device. If a previous one hasn't
* been read yet, the new coredump is discarded. The data lifetime is determined
* by the device coredump framework and when it is no longer needed the @free
* function will be called to free the data.
*/
-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))
+void dev_coredumpm_timeout(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),
+ unsigned long timeout)
{
static atomic_t devcd_count = ATOMIC_INIT(0);
struct devcd_entry *devcd;
@@ -403,7 +407,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
dev_set_uevent_suppress(&devcd->devcd_dev, false);
kobject_uevent(&devcd->devcd_dev.kobj, KOBJ_ADD);
INIT_DELAYED_WORK(&devcd->del_wk, devcd_del);
- schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT);
+ schedule_delayed_work(&devcd->del_wk, timeout);
mutex_unlock(&devcd->mutex);
return;
put_device:
@@ -414,6 +418,32 @@ void dev_coredumpm(struct device *dev, struct module *owner,
free:
free(data);
}
+EXPORT_SYMBOL_GPL(dev_coredumpm_timeout);
+
+/**
+ * dev_coredumpm - create device coredump with read/free methods
+ * @dev: the struct device for the crashed device
+ * @owner: the module that contains the read/free functions, use %THIS_MODULE
+ * @data: data cookie for the @read/@free functions
+ * @datalen: length of the data
+ * @gfp: allocation flags
+ * @read: function to read from the given buffer
+ * @free: function to free the given buffer
+ *
+ * Creates a new device coredump for the given device. If a previous one hasn't
+ * been read yet, the new coredump is discarded. The data lifetime is determined
+ * by the device coredump framework and when it is no longer needed the @free
+ * function will be called to free the data.
+ */
+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);
/**
@@ -55,6 +55,14 @@ static inline void _devcd_free_sgtable(struct scatterlist *table)
void dev_coredumpv(struct device *dev, void *data, size_t datalen,
gfp_t gfp);
+void dev_coredumpm_timeout(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),
+ unsigned long timeout);
+
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,
@@ -72,6 +80,16 @@ static inline void dev_coredumpv(struct device *dev, void *data,
vfree(data);
}
+void dev_coredumpm_timeout(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),
+ unsigned long timeout)
+{
+}
+
static inline void
dev_coredumpm(struct device *dev, struct module *owner,
void *data, size_t datalen, gfp_t gfp,