Re: [PATCH v3 2/4] devcoredump: Add dev_coredumpm_timeout()

[Souza, Jose]

On Mon, 2024-03-25 at 18:00 +0100, Johannes Berg wrote:
> On Mon, 2024-03-04 at 06:39 -0800, José Roberto de Souza wrote:
> > 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.
> 
> > + */
> > +static inline 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))
> 
> nit: looks like you missed a space on the 'free' line
> 
> I don't think we'll actually _solve_ the discussion of whether or not
> this makes sense.
> 
> I still think it's a bad idea to hang on to the dumps in core kernel
> memory since they can be big (I would've said ath12k is big with 55MB,
> but Rodrigo said graphics could be up to 2GB!), without being able to
> page it out, etc. That's just a waste of memory, for what I don't think
> is even a good reason.
> 

There is a discussion to have a udev script to copy dump from memory to disk and then the script can write 0 to coredump/data and erase it from
memory. But if distro don't have this udev script it is still good to have larger timeout to allow user to capture it.

The 2GB usage are for cases when UMD developers enables the capture of every single buffer, regular capture size depends on the complexity of the
application but it is generally round 2MB.

> So dunno.
> 
> However, I also don't like to exercise any power that I might randomly
> hold just because I happened to write the code in the first place... And
> if you want to shoot yourselves in the foot with any of this, should I
> really disagree? I feel I've voiced my objections enough, and Lucas has
> also tried to find ways of making a userspace implementation work for
> you.
> 
> I'd perhaps argue that the documentation for the functions should be
> more opinionated and actually recommend against using a large timeout
> (like you want) for all these reasons, but other than that, the code
> looks fine to me.

@timeout: time in jiffies to remove coredump. DEVCOREDUMP_TIMEOUT is the value for dev_coredumpm() and it should be used unless it is absolutely
necessary a larger timeout.

Or do you have a better suggestion?

> 
> johannes