I'd like to contribute but I don't know how...
Hello, I'd like to contribute to the linux kernel eventually but I'm not sure how, I grabbed a copy of the source code and I found a FIXME that looks like I could fix: /* File: /usr/src/linux/tools/perf/util/string.c * FIXME: replace this with an expression using log10() when we * find a suitable implementation, maybe the one in the dvb drivers... */ I found the implementation they mention, is it okay to just copy paste it into the file? I'm not sure where else to ask this kind of question... Regards, Alex
On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said:
Hello,
I'd like to contribute to the linux kernel eventually but I'm not sure
https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.ht...
how, I grabbed a copy of the source code and I found a FIXME that looks like I could fix:
/* File: /usr/src/linux/tools/perf/util/string.c * FIXME: replace this with an expression using log10() when we * find a suitable implementation, maybe the one in the dvb drivers... */
Step 0: Verify that the comment still matches the code, *and* that the change is still desired. Hint: Why do they want log10()? What does the current code do? What, if anything, will break if you change it? Do you understand *why* '28' was used? And why they didn't just go ahead and use the perfectly usable log10() found in -lm ? For bonus points, understand the code's behavior (or misbehavior) on 32 versus 64 bit architectures, and whether or not that's actually a problem. For extra bonus points, figure out how long that FIXME has been there. And why. (Note that some of these are things that I don't know the answer to offhand. ;)
I found the implementation they mention, is it okay to just copy paste it into the file? I'm not sure where else to ask this kind of question...
If it's going to be used in more than one place, it should be refactored into a function, and both usage sites reworked to call the function rather than inline.
On Wed, Feb 21, 2018 at 10:20 PM, <valdis.kletnieks@vt.edu> wrote:
On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said:
...
I'd like to contribute to the linux kernel eventually but I'm not sure
https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.ht...
This comes up so frequently the list might consider sending a FAQ once a month with links to the answers. Its more useful than Mailman emailing plaintext passwords :) Jeff
On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks@vt.edu wrote:
On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said:
Hello,
I'd like to contribute to the linux kernel eventually but I'm not sure
https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.ht...
how, I grabbed a copy of the source code and I found a FIXME that looks like I could fix:
/* File: /usr/src/linux/tools/perf/util/string.c * FIXME: replace this with an expression using log10() when we * find a suitable implementation, maybe the one in the dvb drivers... */
Step 0: Verify that the comment still matches the code, *and* that the change is still desired. Hint: Why do they want log10()? What does the current code do? What, if anything, will break if you change it?
It's in a function that creates boolean expressions from an array of integers and it looks they hardcoded 28 bytes per each number. I think the intent of using log10() is to only allocate as many bytes as the largest number so expressions with lower numbers(in magnitude) would use less memory.
Do you understand *why* '28' was used? And why they didn't just go ahead and use the perfectly usable log10() found in -lm ?
I'll look into this, I think they are including stdlib.h so it's a very good point. I think they used 28 to be on the safe side 28 bytes gives room for 27 digits per number. They also put the formula they're trying to approximate: /* "%s == %d || " = log10(MAXINT) * 2 + 8 chars for the operators */
For bonus points, understand the code's behavior (or misbehavior) on 32 versus 64 bit architectures, and whether or not that's actually a problem.
I don't think that would be a problem but I have to take a closer look at the implementation of log10().
For extra bonus points, figure out how long that FIXME has been there. And why.
The commit is from 2015, I couldn't find much about it on the lkml.org archives. According the commit message the function's used to generate to generate expressions to filter syscalls when tracing them: perf -e read,write,fork //we only track specified syscalls in this case
(Note that some of these are things that I don't know the answer to offhand. ;)
I found the implementation they mention, is it okay to just copy paste it into the file? I'm not sure where else to ask this kind of question...
If it's going to be used in more than one place, it should be refactored into a function, and both usage sites reworked to call the function rather than inline.
It's only a couple functions, they defined log10() in terms of log2(). I don't really know how to avoid inlining though, I'll look into it.
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On Thu, Feb 22, 2018 at 04:12:14AM +0000, Alex Arvelaez wrote:
On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks@vt.edu wrote:
On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said:
Hello,
I'd like to contribute to the linux kernel eventually but I'm not sure
https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.ht...
how, I grabbed a copy of the source code and I found a FIXME that looks like I could fix:
/* File: /usr/src/linux/tools/perf/util/string.c * FIXME: replace this with an expression using log10() when we * find a suitable implementation, maybe the one in the dvb drivers... */
Step 0: Verify that the comment still matches the code, *and* that the change is still desired. Hint: Why do they want log10()? What does the current code do? What, if anything, will break if you change it?
I went ahead and used log10() from -lm, everything seems to work properly from what I can see. I made a patch with my changes should I send that to the linux-kernel mailing list for feedback? Regards, Alex
On Thu, Feb 22, 2018 at 09:25:39PM +0000, Alex Arvelaez wrote:
On Thu, Feb 22, 2018 at 04:12:14AM +0000, Alex Arvelaez wrote:
On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks@vt.edu wrote:
On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said:
Hello,
I'd like to contribute to the linux kernel eventually but I'm not sure
https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.ht...
how, I grabbed a copy of the source code and I found a FIXME that looks like I could fix:
/* File: /usr/src/linux/tools/perf/util/string.c * FIXME: replace this with an expression using log10() when we * find a suitable implementation, maybe the one in the dvb drivers... */
Step 0: Verify that the comment still matches the code, *and* that the change is still desired. Hint: Why do they want log10()? What does the current code do? What, if anything, will break if you change it?
I went ahead and used log10() from -lm, everything seems to work properly from what I can see. I made a patch with my changes should I send that to the linux-kernel mailing list for feedback?
Documentation/process/ is your friend. Tobin
participants (4)
-
Alex Arvelaez -
Jeffrey Walton -
Tobin C. Harding -
valdis.kletnieks＠vt.edu