Re: I'd like to contribute but I don't know how...

[Tobin C. Harding]

On Thu, Feb 22, 2018 at 09:25:39PM +, Alex Arvelaez wrote:
> On Thu, Feb 22, 2018 at 04:12:14AM +, Alex Arvelaez wrote:
> > On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletni...@vt.edu wrote:
> > > On Thu, 22 Feb 2018 02:33:08 +, 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

___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: I'd like to contribute but I don't know how...

[Alex Arvelaez]

On Thu, Feb 22, 2018 at 04:12:14AM +, Alex Arvelaez wrote:
> On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletni...@vt.edu wrote:
> > On Thu, 22 Feb 2018 02:33:08 +, 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

___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: I'd like to contribute but I don't know how...

[Alex Arvelaez]

On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletni...@vt.edu wrote:
> On Thu, 22 Feb 2018 02:33:08 +, 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@kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies


___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: I'd like to contribute but I don't know how...

[Jeffrey Walton]

On Wed, Feb 21, 2018 at 10:20 PM,   wrote:
> On Thu, 22 Feb 2018 02:33:08 +, 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

___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: I'd like to contribute but I don't know how...

[valdis . kletnieks]

On Thu, 22 Feb 2018 02:33:08 +, 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.


pgp6dQqutTVmu.pgp
Description: PGP signature
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies