Code cleanups are not always simple.

Code cleanups are not always simple.

[Ira Weiny]

Cleaning up code is not always easy.  Yes there are various rules, coding
styles and tools which can automate the process.  But we should all remember to
think about the problem being reported and determine if a simple search/replace
is the proper thing to do.  I think this is becoming more apparent as the
number of things checkpatch reports on is dwindling to these more complicated
issues.

So I'd like to call attention to a couple of problems which have required a bit
more attention.  Hopefully by highlighting these problems folks will be more
aware when looking at future issues.

The first problem was a week or so back when Khadija was trying to fix line
length formatting.[1]  The repeated attempts were causing circular problems
with checkpatch failing on different checks.  The core issue was that
checkpatch was only checking the symptoms of the issue, not the issue itself.
Checkpatch and the kernel coding style are in place not to enforce random rules
on how folks write code.  They are there to make the code cleaner, less buggy,
and easier to maintain.

Today I saw a similar situation.  Julia called out the need to make macro
definitions safer by turning them into inline functions.[2]

But Sumitra took that a bit too literally.[3]  The container_of() macro is
useful on it's own.  The only reason to create additional helper functions is
when the conversion is either more complicated or used so much that the bulk of
the code becomes much much cleaner.

I'll end this with a recent example which is part of the kmap() project.

I was looking at the call to kmap_atomic() in memcpy_page_flushcache() inside
the x86 code.  At first it looks like a simple substitute and replace from
kmap_atmoic() to kmap_local_page().  But I took the time to understand the
context of where memcpy_page_flushcache() was called.  Doing so made me realize
it was not called at all!!!  The proper fix was to simply remove the call
entirely from the kernel.

I know you are all excited to get your first patches into the kernel.  But even
simple patches take time.  After removing these calls I needed to ensure that
x86, powerpc, and arm all compiled without issues.  I used both 'git grep' and
cscope to ensure I removed the header file declarations.  Even then there was
discussion because I accidentally submitted as a series which made a bit more
work for the maintainers.[5]

So in summary take your time.  Try to understand why checkpatch or other tools
are flagging errors.

Most importantly, don't be afraid to take your time when submitting patches.

Ira

[1] https://lore.kernel.org/all/640e75cfd8fc_229a89294a3@iweiny-mobl.notmuch/
[2] https://lore.kernel.org/all/124d2ef9-6f1d-2652-c88d-161f1bc06443@inria.fr/
[3] https://lore.kernel.org/all/ZBh%2Fs5lyONaF37gs@kroah.com/
[4] https://lore.kernel.org/all/20221230-kmap-x86-v1-0-15f1ecccab50@intel.com/
[5] https://lore.kernel.org/all/3523ddf9-03f5-3179-9f39-cec09f79aa97@intel.com/

Re: Code cleanups are not always simple.

[Alison Schofield]

On Mon, Mar 20, 2023 at 05:08:58PM -0700, Ira Weiny wrote:
> Cleaning up code is not always easy.  Yes there are various rules, coding
> styles and tools which can automate the process.  But we should all remember to
> think about the problem being reported and determine if a simple search/replace
> is the proper thing to do.  I think this is becoming more apparent as the
> number of things checkpatch reports on is dwindling to these more complicated
> issues.

Ira,

Thanks for putting words to what I expect applicants and mentors have
been experiencing this round.

Our approach to ramp applicants on the patch submittal process with
trivial cleanups has been challenged. Drivers/staging has few trivial
cleanups where applicants typically developed their patch submittal
skills, before stepping up to solve coding challenges.

Applicants this round are taking giant steps, where previous applicants
took baby steps.

Keep at it,
Alison

> 
> So I'd like to call attention to a couple of problems which have required a bit
> more attention.  Hopefully by highlighting these problems folks will be more
> aware when looking at future issues.
> 
> The first problem was a week or so back when Khadija was trying to fix line
> length formatting.[1]  The repeated attempts were causing circular problems
> with checkpatch failing on different checks.  The core issue was that
> checkpatch was only checking the symptoms of the issue, not the issue itself.
> Checkpatch and the kernel coding style are in place not to enforce random rules
> on how folks write code.  They are there to make the code cleaner, less buggy,
> and easier to maintain.
> 
> Today I saw a similar situation.  Julia called out the need to make macro
> definitions safer by turning them into inline functions.[2]
> 
> But Sumitra took that a bit too literally.[3]  The container_of() macro is
> useful on it's own.  The only reason to create additional helper functions is
> when the conversion is either more complicated or used so much that the bulk of
> the code becomes much much cleaner.
> 
> I'll end this with a recent example which is part of the kmap() project.
> 
> I was looking at the call to kmap_atomic() in memcpy_page_flushcache() inside
> the x86 code.  At first it looks like a simple substitute and replace from
> kmap_atmoic() to kmap_local_page().  But I took the time to understand the
> context of where memcpy_page_flushcache() was called.  Doing so made me realize
> it was not called at all!!!  The proper fix was to simply remove the call
> entirely from the kernel.
> 
> I know you are all excited to get your first patches into the kernel.  But even
> simple patches take time.  After removing these calls I needed to ensure that
> x86, powerpc, and arm all compiled without issues.  I used both 'git grep' and
> cscope to ensure I removed the header file declarations.  Even then there was
> discussion because I accidentally submitted as a series which made a bit more
> work for the maintainers.[5]
> 
> So in summary take your time.  Try to understand why checkpatch or other tools
> are flagging errors.
> 
> Most importantly, don't be afraid to take your time when submitting patches.
> 
> Ira
> 
> [1] https://lore.kernel.org/all/640e75cfd8fc_229a89294a3@iweiny-mobl.notmuch/
> [2] https://lore.kernel.org/all/124d2ef9-6f1d-2652-c88d-161f1bc06443@inria.fr/
> [3] https://lore.kernel.org/all/ZBh%2Fs5lyONaF37gs@kroah.com/
> [4] https://lore.kernel.org/all/20221230-kmap-x86-v1-0-15f1ecccab50@intel.com/
> [5] https://lore.kernel.org/all/3523ddf9-03f5-3179-9f39-cec09f79aa97@intel.com/
> 

Re: Code cleanups are not always simple.

[Julia Lawall]

> Most importantly, don't be afraid to take your time when submitting patches.

Thanks Ira.

But don't be afraid to mess up either :)  We're here to help correct
mistakes and help you (and everyone else) learn. There are a lot of little
details, and it is hard to think of everything in advance.  This point of
the application period is that thinking of all of these issues become
second nature, so that you can interact with the community during your
project in a productive way.

julia

Re: Code cleanups are not always simple.

[Sumitra Sharma]

Hi Ira,

I will surely going to take care of the things you mentioned here. Thank you.

Regards,
Sumitra

Re: Code cleanups are not always simple.

[Khadija Kamran]

On Mon, Mar 20, 2023 at 05:08:58PM -0700, Ira Weiny wrote:
> Cleaning up code is not always easy.  Yes there are various rules, coding
> styles and tools which can automate the process.  But we should all remember to
> think about the problem being reported and determine if a simple search/replace
> is the proper thing to do.  I think this is becoming more apparent as the
> number of things checkpatch reports on is dwindling to these more complicated
> issues.
> 
> So I'd like to call attention to a couple of problems which have required a bit
> more attention.  Hopefully by highlighting these problems folks will be more
> aware when looking at future issues.
> 
> The first problem was a week or so back when Khadija was trying to fix line
> length formatting.[1]  The repeated attempts were causing circular problems
> with checkpatch failing on different checks.  The core issue was that
> checkpatch was only checking the symptoms of the issue, not the issue itself.
> Checkpatch and the kernel coding style are in place not to enforce random rules
> on how folks write code.  They are there to make the code cleaner, less buggy,
> and easier to maintain.
> 
> Today I saw a similar situation.  Julia called out the need to make macro
> definitions safer by turning them into inline functions.[2]
> 
> But Sumitra took that a bit too literally.[3]  The container_of() macro is
> useful on it's own.  The only reason to create additional helper functions is
> when the conversion is either more complicated or used so much that the bulk of
> the code becomes much much cleaner.
> 
> I'll end this with a recent example which is part of the kmap() project.
> 
> I was looking at the call to kmap_atomic() in memcpy_page_flushcache() inside
> the x86 code.  At first it looks like a simple substitute and replace from
> kmap_atmoic() to kmap_local_page().  But I took the time to understand the
> context of where memcpy_page_flushcache() was called.  Doing so made me realize
> it was not called at all!!!  The proper fix was to simply remove the call
> entirely from the kernel.
> 
> I know you are all excited to get your first patches into the kernel.  But even
> simple patches take time.  After removing these calls I needed to ensure that
> x86, powerpc, and arm all compiled without issues.  I used both 'git grep' and
> cscope to ensure I removed the header file declarations.  Even then there was
> discussion because I accidentally submitted as a series which made a bit more
> work for the maintainers.[5]
> 
> So in summary take your time.  Try to understand why checkpatch or other tools
> are flagging errors.
> 
> Most importantly, don't be afraid to take your time when submitting patches.
> 
> Ira
> 
> [1] https://lore.kernel.org/all/640e75cfd8fc_229a89294a3@iweiny-mobl.notmuch/
> [2] https://lore.kernel.org/all/124d2ef9-6f1d-2652-c88d-161f1bc06443@inria.fr/
> [3] https://lore.kernel.org/all/ZBh%2Fs5lyONaF37gs@kroah.com/
> [4] https://lore.kernel.org/all/20221230-kmap-x86-v1-0-15f1ecccab50@intel.com/
> [5] https://lore.kernel.org/all/3523ddf9-03f5-3179-9f39-cec09f79aa97@intel.com/
>

Hey Ira,

Thank you for the mail. I will make sure to keep your instructions in
mind while working on the upcoming patches.

Thank you,
Regards,
Khadija

Thank you