SoylentNews is people
SoylentNews
from the Budget-Fair-Queueing-AKA-getting-in-line-at-a-cheap-carnival? dept.
For years the BFQ (Budget Fair Queueing) I/O scheduler has been trying to get in the mainline kernel and it looks like they have an action plan for getting accepted upstream.
BFQ is a proportional-share I/O scheduler that shares a lot of code with the CFQ scheduler. The Completely Fair Queuing (CFQ) scheduler has long been part of the mainline tree but BFQ hasn't been pulled yet even after many revisions and code reviews. Despite that, it is used as a default I/O scheduler on several Linux distributions, such as Manjaro, OpenMandriva, Sabayon, or CyanoGenMod, for some devices.
While it doesn't look like it will be ready for the upcoming Linux 4.2 cycle, it appears BFQ getting accepted is becoming quite close (a Google Groups link).
A direct link to relevant lkml thread is http://lkml.org/lkml/2015/6/5/822.
Original Submission
(Score: 5, Interesting) by FatPhil on Wednesday June 10 2015, @03:07PM
And you know what, I don't think this patch passes the sniff test:
blkcg = kzalloc(sizeof(*blkcg), GFP_KERNEL);
- if (!blkcg)
- return ERR_PTR(-ENOMEM);
+ if (!blkcg) {
+ ret = ERR_PTR(-ENOMEM);
+ goto free_blkcg;
+ }
... irrelevant what's here, except to say that there are no other references to the "free_blkcg" label.
+free_blkcg:
+ kfree(blkcg);
+ return ret;
Why? Just Why? There's no functional difference to that code apart from you stashing the return value into a variable and forcing a call to the kfree function with a NULL pointer, so it will return immediately. If you really prefer error-path forward gotos to early returns (and there's no preference for either in the kernel at large), then at least make the label you jump to be more sensibly located. But as I say, this is a cosmetic change, make purely out of personal stylistic preferences, and is unrelated to the patch that actually adds the new functionality. (And therefore should be a separate patch.)
I have no idea if the rest of the code is as amateurish, but that alone would have make me ask for a v2.
[* Not strictyle true; corporations being corporations, there would be some BFQ managers who ran to thir boss' boss' boss to demand that their team's shitty patches were included after we'd rejected them 8 times.]
Great minds discuss ideas; average minds discuss events; small minds discuss people; the smallest discuss themselves
(Score: 2, Informative) by Anonymous Coward on Wednesday June 10 2015, @03:16PM
There's a fallthrough from the cleanup code free_pd_blkcg immediately above free_blkcg. So the code labeled 'free_blkcg' is being used in more than one place.
I've coded mostly in C++ for the past 25 years, so I'm not a huge fan of the goto style of function call cleanup, but it's apparently the most popular way to do to things in large C projects where RAII is not available. Returning from a conditional block inside a function should be avoided, except for small functions of about 10 LOC or less.
(Score: 4, Informative) by Thexalon on Wednesday June 10 2015, @03:48PM
I worked for a guy that insisted on avoiding early returns from functions. Apparently, a lot of schools of thought teach that early returns are the Wrong Thing and to avoid them at all costs. "All costs" includes but is not limited to:
* More code.
* More complex code (e.g. if (foo) { return bar; } ... rest of function ... versus if (foo) { retval = bar; } else { ... rest of function ... return retval; })
* Longer commits (as in this situation)
I don't know what the rules are in kernel space, but I'm fairly certain whoever it is that did this didn't do it because they were amateurish, but rather because they were taught that it was the One True Way. As long as it follows the cardinal rule of kernel hacking ("Thou shalt not break userspace"), I'm fine with it, even though it's not really my style.
The only thing that stops a bad guy with a compiler is a good guy with a compiler.
(Score: 0) by Anonymous Coward on Wednesday June 10 2015, @03:58PM
If the function has cleanup code at the end then early returns should be avoided, except at the very beginning (arg isn't of the sort we're interested in - goodbye).
A good use of an early return would be a "find" function that iterates a collection, then it might as well return as soon as there's a hit. At the bottom of the function, there will be a "return NULL" or "return false". But those are dinky little functions.
(Score: 4, Insightful) by mechanicjay on Wednesday June 10 2015, @05:10PM
My problem with early returns is that it can lead to unintended results if you're not careful, especially in more complex functions. I find they also tend to make the code harder to read, which is a maintainability issue, especially when I'm the next guy.
I personally follow the convention of ONE return statement per function, all the logic through that function should boil down to something reasonable and return it. Agreed on the possible exception of failing input validation.
My VMS box beat up your Windows box.
(Score: 2) by Nerdfest on Wednesday June 10 2015, @10:02PM
I agree, and though I used to be a "one return per method" zealot, now I use early returns in the case where *it actually makes the code easier to read*. Small functions, clearly formatted, and to help avoid unnecessary indentation.
(Score: 2) by DarkMorph on Wednesday June 10 2015, @11:00PM
Alternatively we can consider another legitimate case where there are two or more successful returns. An easy example is your typical success case and a "passive" success where there is no failure, but also nothing special to do.
In short I would argue that early returns/multiple return points are not necessarily an indication of bad design nor difficult to follow logic flow.
(Score: 3, Informative) by FatPhil on Wednesday June 10 2015, @06:00PM
I used to be very much "no gotos". I've grown out of that hangup now. (Which could have been because of the many years working on the Linux kernel.)
Great minds discuss ideas; average minds discuss events; small minds discuss people; the smallest discuss themselves
(Score: 3, Informative) by HiThere on Wednesday June 10 2015, @06:43PM
Go tos are a lot less harmful if they are used only sparsely. The "Go to considered harmful" paper was written before constructs like blocks following a test were added to languages.
Similarly, the rule about single returns (except for failing validity checks) is more important if your functions are long. For something less than around 30 lines long it hardly matters, because you can see the whole thing at once.
(I haven't checked this particular case, and I do reflexively avoid gotos, but then I also don't use much C any more. I consider unrestricted pointer use as dangerous, or more, than unrestricted "go to" use.)
IIRC, the paper about "Go to considered harmful" was written about Fortran IV, advocating a more structured approach, subsequently adopted by all languages (except assembler). When I remember the spaghetti code I used to deal with I agree with it completely, but wild pointers are just as bad.
All that said, sometimes you just need to use the best tool available even if it is dangerous. But if it's dangerous you should strictly limit your use.
Javascript is what you use to allow unknown third parties to run software you have no idea about on your computer.
(Score: 2) by frojack on Wednesday June 10 2015, @08:09PM
IIRC, the paper about "Go to considered harmful" was written about Fortran IV,
As I read it, Dijkstra was using pseudo code to make his point, and I don't believe there was any intentional language reference.
At the time it was written (1968) the structure of code segments in source code was already starting to get attention because the tools for handling large code collections were primitive at best, and carrying a concept of the structure in one's head was becoming increasingly difficult. As well, the hardware of the day had limitations making rampant branching really inefficient.
But it didn't matter, as It really affected ALL code regardless of language once you got beyond some very small size of programs.
As usual, the no GO TO mantra was followed far too religiously, more or less to get people to think about what they were doing.
The inevitable result was the tongue in cheek recommendation of the COME FROM statement.
No, you are mistaken. I've always had this sig.
(Score: 3, Funny) by maxwell demon on Wednesday June 10 2015, @08:13PM
I'm pretty sure you wanted to put the closing brace of the else branch before the return statement. ;-)
The Tao of math: The numbers you can count are not the real numbers.
(Score: 2) by dltaylor on Wednesday June 10 2015, @09:13PM
I fought with that mentality a few times. I came up with a solution that is reasonably clean, although it uses more labels.
As resources were allocated, state variables set, hardware registers (in the case of drivers) set, I know that things need to be unwound in roughly reverse order, so the labels were placed to only unwind what had definitely been "wound". Only parameter checking could early exit, and the return of the function was initially set to something like ERR_PTR(-EIO) or whatever was appropriate for that type of function, and only set good when it truly was. In this way, all of the steps from the label down needed to be executed, you didn't make useless calls to kfree(), for example. Some drivers have had 5 or 6 labels, but it eliminates the "if (previous_action_successful) unwind(previous_action);" checking during the exit, and I didn't need to keep extraneous state, since the label was, effectively, state.
(Score: 0) by Anonymous Coward on Thursday June 11 2015, @07:41AM
That's all fine, but in this case, the check was that no memory had been allocated, and the goto was to the line free'ing the resulting null pointer.