On Tue, May 22, 2012 at 02:17:06PM -0700, Tejun Heo wrote:> Hello, Kent.> > I've been reading the code and am still very far from understanding> the whole thing but here are general impressions upto this point.> > * I started reading from get_bucket(), mostly because I wasn't sure> where to start. It would be very helpful to the reviewers and> future participants if there's a design doc and more comments (much> more). The code is on the big side and the problem is exacerbated> by use of somewhat unconventional constructs and conventions and> lack of documentation and comments.

Yeah, I'm gonna try and spend more time on documentation. That's gonnatake a bit, though.

> * It's unfortunate but true that different parts of kernel are held to> different standards in terms of conformance to overall style and> structure. Code for obscure device, for example, is allowed to be> way more weird than common core code. That said, one thing which is> generally frowned upon is bringing in lots of foreign constructs> because that tends to make the code much more difficult to follow> for other people than plain silly code. Even for drivers, code with> a lot of wrappers and foreign constructs (synchronization, data> structure...) tend to get nacked and are recommended to convert to> existing infrastructure even if that ends up, say, more verbose> code.> > It is also true that new constructs and mechanisms are constantly> being added to kernel and the ones used by bcache could be useful> enough to have; however, with my currently limited understanding,> the oddities seem too much.> > * Too many smart macros. Macros suck. Smart ones double suck.> > * For file internal ones, it's okay to be somewhat relaxed with> namespace separation but bcache code seems to be going too far. It> also becomes a lot worse with macros as warnings and errors from> compiler get much more difficult to decipher. e.g. things like> next() and end() macros are calling for trouble.

Yes, that's been on the todo list forever - legacy of it starting out asa single file. I'll try and work some more on namespacing tonight.

> * Somewhat related to the above, I'm not a fan of super-verbose> symbols but I *hope* that the variable names were just a tiny bit> more descriptive. At least, the ones used as arguments.

If you want to point out particularly egregious examples in your codereviews I'll tackle those first. I suffer from being too close to thecode, so I really have a hard time guessing what's going to trip otherpeople up.

> * The biggest thing that I dislike about closure is that it's not> clear what it does when I see one. Is it a refcount,> synchronization construct or asynchronous execution mechanism? To> me, it seems like a construct with too many purposes and too much> abstraction, which tends to make it difficult to understand, easy to> misuse and difficult to debug. IMHO, constructs like this could> seem very attractive to small group of people with certain concepts> firmly on their minds; however, one man's paradise is another man's> hell and we're often better off without either. In many ways,> closure feels like kobject and friends.

Heh, it's worse than kobjects in that respect. But I really like thefact that it's more abstract than kobjects, in that it's not at all tiedto implementation details of sysfs or anything else.

> I'd like to recommend using something more concerete even if that> means more verbosity. ie. if there are lots of bio sequencing going> on, implement and use bio sequencer. That's way more concerete and> concerete stuff tends to be much easier to wrap one's head around> mostly because it's much easier to agree upon for different people> and they don't have to keep second-guessing what the original author> would have on his/her mind while implementing it.

There's got to be some kind of middle ground. Having a unifyingabstraction for refcounty things really is incredibly powerful - theunified parent mechanism makes a lot of problems go away, I don't thinkI can adequately explain how sucessful that was, you had to experienceit.

But nevertheless I sympathize with your basic complaint - it ought to bepossible to restructure something so the code is easier to follow andit's easier to tell what a given closure is for.

Maybe if I go through and document all the instances where they'redeclared it might help.

Had not come across memparse(). I don't think it's a sufficientreplacement for my code because of the per type limits, but we shoulddefinitely get rid of the duplication one way or the other.

As you can probably tell I'm strongly biased against duplication whenit's a question of macros vs. duplication - but if we can make thecommend backend code + wrapper approach work that's even better (smallercode size ftw). It'll be tricky to get u64 vs. int64 right, but I'lltake a stab at it.

Yeah, but considering the output is very bounded... you are technicallycorrect, but I have a hard time caring. Making it a vsnprintf modifierwould make that issue go away though. I'll see if I can figure out howto do it.

I don't really get your objection to jumping into the middle of loops...sure it shouldn't be the first way you try to write something, but Idon't see anything inherently wrong with it. IMO it _can_ make the codeeasier to follow.

No important difference, I just never saw bitmap_weight() before - I'llswitch to that. (These are faster, but bigger and in the kernel I highlydoubt we've a workload where the performance of popcount justifies theextra memory).

I have a passionate... dislike of templates, but do you have anyalternatives? I don't know any other way of implementing this thatdoesn't give up typechecking, and especially for the fifo code thattypechecking is just too important to give up.