On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks@xxxxxx 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.html > > > 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@xxxxxxxxxxxxxxxxx > https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
