* I'd like to contribute but I don't know how...
@ 2018-02-22 2:33 Alex Arvelaez
2018-02-22 3:20 ` valdis.kletnieks at vt.edu
0 siblings, 1 reply; 6+ messages in thread
From: Alex Arvelaez @ 2018-02-22 2:33 UTC (permalink / raw)
To: kernelnewbies
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how...
2018-02-22 2:33 I'd like to contribute but I don't know how Alex Arvelaez
@ 2018-02-22 3:20 ` valdis.kletnieks at vt.edu
2018-02-22 3:46 ` Jeffrey Walton
2018-02-22 4:12 ` Alex Arvelaez
0 siblings, 2 replies; 6+ messages in thread
From: valdis.kletnieks at vt.edu @ 2018-02-22 3:20 UTC (permalink / raw)
To: kernelnewbies
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?
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 486 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180221/0771468f/attachment-0001.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how...
2018-02-22 3:20 ` valdis.kletnieks at vt.edu
@ 2018-02-22 3:46 ` Jeffrey Walton
2018-02-22 4:12 ` Alex Arvelaez
1 sibling, 0 replies; 6+ messages in thread
From: Jeffrey Walton @ 2018-02-22 3:46 UTC (permalink / raw)
To: kernelnewbies
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.html
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how...
2018-02-22 3:20 ` valdis.kletnieks at vt.edu
2018-02-22 3:46 ` Jeffrey Walton
@ 2018-02-22 4:12 ` Alex Arvelaez
2018-02-22 21:25 ` Alex Arvelaez
1 sibling, 1 reply; 6+ messages in thread
From: Alex Arvelaez @ 2018-02-22 4:12 UTC (permalink / raw)
To: kernelnewbies
On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks at 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.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 at kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how...
2018-02-22 4:12 ` Alex Arvelaez
@ 2018-02-22 21:25 ` Alex Arvelaez
2018-02-23 23:11 ` Tobin C. Harding
0 siblings, 1 reply; 6+ messages in thread
From: Alex Arvelaez @ 2018-02-22 21:25 UTC (permalink / raw)
To: 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 at 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.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?
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* I'd like to contribute but I don't know how...
2018-02-22 21:25 ` Alex Arvelaez
@ 2018-02-23 23:11 ` Tobin C. Harding
0 siblings, 0 replies; 6+ messages in thread
From: Tobin C. Harding @ 2018-02-23 23:11 UTC (permalink / raw)
To: kernelnewbies
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 at 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.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?
> >
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-23 23:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-22 2:33 I'd like to contribute but I don't know how Alex Arvelaez
2018-02-22 3:20 ` valdis.kletnieks at vt.edu
2018-02-22 3:46 ` Jeffrey Walton
2018-02-22 4:12 ` Alex Arvelaez
2018-02-22 21:25 ` Alex Arvelaez
2018-02-23 23:11 ` Tobin C. Harding
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).