Re: [RFCv11 PATCH 04/29] media-request: core request support

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Wed, 11 Apr 2018 18:02:19 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> On Wed, Apr 11, 2018 at 10:49:35AM -0300, Mauro Carvalho Chehab wrote:
> > Em Wed, 11 Apr 2018 16:21:16 +0300
> > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> > 
> >   
> > > > > > Btw, this is a very good reason why you should define the ioctl to
> > > > > > have an integer argument instead of a struct with a __s32 field
> > > > > > on it, as per my comment to patch 02/29:
> > > > > > 
> > > > > > 	#define MEDIA_IOC_REQUEST_ALLOC	_IOWR('|', 0x05, int)
> > > > > > 
> > > > > > At 64 bit architectures, you're truncating the file descriptor!      
> > > > > 
> > > > > I'm not quite sure what do you mean. int is 32 bits on 64-bit systems as
> > > > > well.    
> > > > 
> > > > Hmm.. you're right. I was thinking that it could be 64 bits on some
> > > > archs like sparc64 (Tru64 C compiler declares it with 64 bits), but,
> > > > according with:
> > > > 
> > > > 	https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html
> > > > 
> > > > This is not the case on gcc.    
> > > 
> > > Ok. The reasoning back then was that what "int" means varies across
> > > compilers and languages. And the intent was to codify this to __s32 which
> > > is what the kernel effectively uses.  
> > 
> > ...
> >   
> > > The rest of the kernel uses int rather liberally in the uAPI so I'm not
> > > sure in the end whether something desirable was achieved. Perhaps it'd be
> > > good to go back to the original discussion to find out for sure.
> > > 
> > > Still binaries compiled with Tru64 C compiler wouldn't work on Linux anyway
> > > due to that difference.
> > > 
> > > Well, I stop here for this begins to be off-topic. :-)  
> > 
> > Yes. Let's keep it as s32 as originally proposed. Just ignore my comments
> > about that :-)
> >   
> > > > > > > +	get_task_comm(comm, current);
> > > > > > > +	snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
> > > > > > > +		 comm, fd);      
> > > > > > 
> > > > > > Not sure if it is a good idea to store the task that allocated
> > > > > > the request. While it makes sense for the dev_dbg() below, it
> > > > > > may not make sense anymore on other dev_dbg() you would be
> > > > > > using it.      
> > > > > 
> > > > > The lifetime of the file handle roughly matches that of the request. It's
> > > > > for debug only anyway.
> > > > > 
> > > > > Better proposals are always welcome of course. But I think we should have
> > > > > something here that helps debugging by meaningfully making the requests
> > > > > identifiable from logs.    
> > > > 
> > > > What I meant to say is that one PID could be allocating the
> > > > request, while some other one could be actually doing Q/DQ_BUF.
> > > > On such scenario, the debug string could provide mislead prints.    
> > > 
> > > Um, yes, indeed it would no longer match the process. But the request is
> > > still the same. That's actually a positive thing since it allows you to
> > > identify the request.
> > > 
> > > With a global ID space this was trivial; you could just print the request
> > > ID and that was all that was ever needed. (I'm not proposing to consider
> > > that though.)
> > >   
> > 
> > IMO, a global ID number would work better than get_task_comm().
> > 
> > Just add a static int monotonic counter and use it for the debug purposes,
> > e. g.:
> > 
> > {
> > 	static unsigned int req_count = 0;
> > 
> > 	snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
> > 		req_count++, fd);    
> > 
> > Ok, eventually, it will overflow, but, it will be unique within
> > a reasonable timeframe to be good enough for debugging purposes.  
> 
> Yes, but you can't figure out which process allocated it anymore, making
> associating kernel debug logs with user space process logs harder.
> 
> How about process id + file handle? That still doesn't tell which process
> operated on the request though, but I'm not sure whether that's really a
> crucial piece of information.

You don't need that. With dev_dbg() - and other *_dbg() macros - you can
enable process ID for all debug messages.

Basically, if the user needs the PID, all it needs is to use "+pt",
e. g. something like:

	echo "file drivers/media/* +pt" > /sys/kernel/debug/dynamic_debug/control


[1] see:
	https://www.kernel.org/doc/html/v4.11/admin-guide/dynamic-debug-howto.html

Thanks,
Mauro