Bug 240545 - patch to remove unneeded M_WAITOK return value checks
Summary: patch to remove unneeded M_WAITOK return value checks
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-09-12 21:28 UTC by Andrew Reiter
Modified: 2019-10-08 22:20 UTC (History)
2 users (show)

See Also:


Attachments
remove NULL check and handling of it for M_WAITOK cases (11.46 KB, patch)
2019-09-12 21:28 UTC, Andrew Reiter
no flags Details | Diff
Coccinelle patch that generates a unified diff for this (42.49 KB, text/plain)
2019-10-06 15:31 UTC, Andrew Reiter
no flags Details
Cocci 2 (21.78 KB, text/plain)
2019-10-07 20:40 UTC, Andrew Reiter
no flags Details
updated patch with better coverage (27.74 KB, patch)
2019-10-08 02:51 UTC, Andrew Reiter
no flags Details | Diff
Final (?) source diff with a couple cases added that were missed by coccinelle (28.31 KB, patch)
2019-10-08 22:20 UTC, Andrew Reiter
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Reiter 2019-09-12 21:28:35 UTC
Created attachment 207432 [details]
remove NULL check and handling of it for M_WAITOK cases

Attached is a patch that removes the check of malloc(9)'s return value against NULL when M_WAITOK flag is used. This does not cover all cases, but a number of them that I found with an LLVM pass and partial compilation of kernel. That pass also checks mallocarray(), realloc(), and reallocf(), but in the areas analyzed did not see the extra code. I am not sure I like the idea of removing NULL checks, but it is already handled. Are there any cases to be aware of for why the check would be necessary with M_WAITOK? If no, I will try to go through and look for others. The patch is for 12.0-RELEASE, but can adjust that as well.
Comment 1 Conrad Meyer freebsd_committer 2019-09-12 21:38:09 UTC
It is definitely ok for malloc, mallocarray, and realloc.  (We have a kernel reallocf?  Huh.)  Honestly, reallocf with M_WAITOK seems like a mistake — those could instead/additionally be replaced with just realloc(M_WAITOK).

> Are there any cases to be aware of for why the check would be necessary with M_WAITOK?

Yes, contigmalloc() can fail with M_WAITOK; some of the busdma allocation stuff can fail with M_WAITOK.  Normal malloc / UMA allocations with M_WAITOK can't return NULL (just sleep indefinitely).
Comment 2 Conrad Meyer freebsd_committer 2019-09-12 21:39:22 UTC
By the way, have you considered writing this as a coccinelle script?  It seems like something that could be done with that tool.
Comment 3 Andrew Reiter 2019-09-12 21:49:11 UTC
(In reply to Conrad Meyer from comment #2)
No I have not used that tool before, but it looks handy. Is there any history of it being used in any FreeBSD processes before? Thanks for the pointer!
Comment 4 Conrad Meyer freebsd_committer 2019-09-12 22:27:51 UTC
Yes, I've used it, pfg@ has used it some, and I believe mjg@ has as well (https://reviews.freebsd.org/D21427).
Comment 5 Andrew Reiter 2019-09-13 19:36:05 UTC
Comment for posterity: the pass used to find the ones in patch: https://github.com/roachspray/mwaitok-unneeded-rv-check
Comment 6 Andrew Reiter 2019-10-05 03:22:50 UTC
Brief update: I have worked up a Coccinelle semantic patch for this, finally. I am going to do local testing and then push it here.
Comment 7 Andrew Reiter 2019-10-06 15:31:11 UTC
Created attachment 208142 [details]
Coccinelle patch that generates a unified diff for this

Still need to look through / basic test the generated patch, but wanted to share the Coccinelle spatch. It could probably be simplified, but seems to have found a number of cases. It took a long time (at least 12 hours) to run on the src/sys tree.
Comment 8 Conrad Meyer freebsd_committer 2019-10-06 18:36:44 UTC
There's some way of declaring that a coccinelle variable ('buf') hasn't been reassigned since the earlier malloc call, although I don't recall how to do it.  E.g., in this code,

  buf = malloc(M_WAITOK);
  ...
  buf = malloc(M_NOWAIT);
  if (buf == NULL) {
    /* this case shouldn't be removed */
  }

The null check shouldn't be removed.

It's a weird and probably bad pattern to reuse the same C variable for allocation with a different policy in the same function, but possible.

> Still need to look through / basic test the generated patch, but wanted to share the Coccinelle spatch.

Thanks!

> It took a long time (at least 12 hours) to run on the src/sys tree.

Yeah, it's not fast.
Comment 9 Andrew Reiter 2019-10-06 20:09:14 UTC
(In reply to Conrad Meyer from comment #8)
Ahh, really good point; thank you for bringing that up. I will look into this.
Comment 10 Conrad Meyer freebsd_committer 2019-10-06 20:29:32 UTC
Also, I think the patch could be simplified quite a bit if we can nest the conditional matching (i.e., result cast to type or not; position of M_WAITOK in flags; even allocation function (not sure the number of arguments matter, flags is always the last one).  Do the conditional expressions ("( | )" pattern) not nest?
Comment 11 Andrew Reiter 2019-10-07 20:40:25 UTC
(In reply to Conrad Meyer from comment #10)
Thanks for the comments. I am attaching a new version (titled "cocci 2" in the attachments list) that simplifies some things. It probably could be a lot better, but I am new to Coccinelle. I think this may be my last round of futzing with the Coccinelle part and will start to look at the changes it results in after it runs locally. Just wanted to get a newer version up to share.
Comment 12 Andrew Reiter 2019-10-07 20:40:57 UTC
Created attachment 208157 [details]
Cocci 2
Comment 13 Andrew Reiter 2019-10-07 21:14:25 UTC
(In reply to Andrew Reiter from comment #12)
And this version runs in a reasonable amount of time :-P
Comment 14 Conrad Meyer freebsd_committer 2019-10-08 01:01:04 UTC
(In reply to Andrew Reiter from comment #11)
> Thanks for the comments. I am attaching a new version (titled "cocci 2" in the
> attachments list) that simplifies some things.

Thanks!

> It probably could be a lot better, but I am new to Coccinelle.

No worries, it's new to me as well.  :-)

> And this version runs in a reasonable amount of time :-P

Even better :-).
Comment 15 Andrew Reiter 2019-10-08 02:51:22 UTC
Created attachment 208169 [details]
updated patch with better coverage

Ok, so the cocci 2 script seems to generate some FP cases that I had to change and this is expressed in this attachment. There may be some other cases not found ... seems braces would play a factor (i.e., match on 'if () statement'- like cases. I scanned the patch and seemed to see the cases that were being handled. The cases that seemed questionable, I looked and adjusted as needed. Tested with LINT and GENERIC... not much testing though.
Comment 16 Andrew Reiter 2019-10-08 04:30:03 UTC
(In reply to Andrew Reiter from comment #15)
IOW, I think the last diff attached may be worth going forward with.
Comment 17 Andrew Reiter 2019-10-08 22:20:12 UTC
Created attachment 208185 [details]
Final (?) source diff with a couple cases added that were missed by coccinelle

Final (?) source diff with a couple cases added that were missed by coccinelle that I hand fixed.