Re: [PATCH 10/30] staging: nvec: Introduce new internal API for msg alloc/free

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Sep 24, 2011 at 04:57:13AM +0300, Dan Carpenter wrote:
> What I'm trying to explain is that kernel style is minimalist.  Focus
> on writing the simplest, most readable code you can.  Kernel coders
> are 10 times more likely to use "int i" than they are to use
> "unsigned int i".
> 
> $ git grep "       int i;" | wc -l
> 13956
> $ git grep "       unsigned int i;" | wc -l
> 1508
> 
> "int" is the default number in C and "unsigned int" has a modifier
> and so it's a tiny bit special.  When you use "unsigned int" you're
> drawing attention to your variable because it is not just a normal
> number.  Someone reviewing that code immediately thinks, "How high
> is NVEC_POOL_SIZE exactly?"  Certainly that was my thought when I
> saw it declared as a size_t.

That's interesting. For me it's the opposite, when I see someone
counting the length of an array in something different than size_t,
I wonder why they're doing it; as size_t is the type specifically
meaning size or count of in-memory objects (in the kernel case,
that was kfifo I wondered about, or most kernel code in
general) -- Likewise, why there is no irqflags_t type, that
would have prevented various bugs that occured in the past
when people used unsigned int instead of unsigned long, and
would make it clear that the variable stores those flags, not
random flags for other stuff.

The size_t was not really intended to get here anyway after I
noticed that, but I somehow forgot it then.


> But in fact the for loop is a boring ordinary for loop.  You've
> tricked the reviewers, and you've distracted them from the 5 second
> sleep elsewhere in your code.

A 5 second timeout, and that's mostly not reached. 5 seconds seems to
long, but I have not rewritten that part of the code yet (I once
tried 100 ms, but that did not work and in the end just caused
the EC to not respond anymore). The workers also have other bugs:

	(a) If I queue two writes, I queue two workers, the first
	    one then processes two writes, and the second just burns
	    cpu cycles for nothing
	(b) Event processing is mixed with general response processing
	    which can lead to sticky keys, as the release event gets
	    delayed

I'd like to rework them at some point, but for now I kept them as
they were in Marc's patch from last month. I can't do everything
in a single week.

> 
> Obviously, I'm exaggerating a bit and in this case it doesn't matter
> one way or the other.  Go ahead and declare it as unsigned so long
> as you understand about kernel style.  Don't be clever, fancy or
> special.  Don't optimize.  Don't stand out.  And don't do more work
> than you have to.

I also exaggerated a bit, probably because it was already past
midnight. I shouldn't write emails past midnight.

-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux