Re: [PATCH 5/9] lib/ref_tracker: improve allocation flags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 17.02.2022 16:13, Eric Dumazet wrote:
On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@xxxxxxxxx> wrote:
Library can be called in non-sleeping context, so it should not use
__GFP_NOFAIL. Instead it should calmly handle allocation fails, for
this __GFP_NOWARN has been added as well.
Your commit changelog is misleading .

The GFP_NOFAIL issue has been fixed already in
commit c12837d1bb31032bead9060dec99ef310d5b9fb7 ("ref_tracker: use
__GFP_NOFAIL more carefully")

I based the patchset on drm-tip, which do not have this commit, I will take a look how to keep drm-tip base (to allow intel CI tests) and take patch above into account - maybe simple cherry-picking?



Signed-off-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
---
  lib/ref_tracker.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index 7b00bca300043..c8441ffbb058a 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -59,7 +59,7 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir,
         if (list_empty(&dir->list))
                 return;

-       sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT);
+       sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT | __GFP_NOWARN);
This belongs to patch 3 in your series.

OK, again historical reason.


         list_for_each_entry(tracker, &dir->list, head)
                 ++total;
@@ -154,11 +154,11 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir,
         unsigned long entries[REF_TRACKER_STACK_ENTRIES];
         struct ref_tracker *tracker;
         unsigned int nr_entries;
-       gfp_t gfp_mask = gfp;
Simply change this line to : gfp_t gfp_mask = gfp | __GFP_NOFAIL;

and "| __GFP_NOWARN".


+       gfp_t gfp_mask;
         unsigned long flags;

Then leave all this code as is ? I find current code more readable.

Yep you are right.


-       if (gfp & __GFP_DIRECT_RECLAIM)
-               gfp_mask |= __GFP_NOFAIL;
+       gfp |= __GFP_NOWARN;
+       gfp_mask = (gfp & __GFP_DIRECT_RECLAIM) ? (gfp | __GFP_NOFAIL) : gfp;
         *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask);
         if (unlikely(!tracker)) {
                 pr_err_once("memory allocation failure, unreliable refcount tracker.\n");
@@ -191,7 +191,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir,
         }
         nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1);
         nr_entries = filter_irq_stacks(entries, nr_entries);
lib/ref_tracker.c got patches in net-next, your patch series is going
to add conflicts.

git log --oneline 5740d0689096..4d449bdc5b26 --no-merges -- lib/ref_tracker.c
c2d1e3df4af59261777b39c2e47476acd4d1cbeb ref_tracker: remove
filter_irq_stacks() call
8fd5522f44dcd7f05454ddc4f16d0f821b676cd9 ref_tracker: add a count of
untracked references
e3ececfe668facd87d920b608349a32607060e66 ref_tracker: implement
use-after-free detection


So I will cherry-pick these patches into next version of patchset, with "NO MERGE" annotation (to allow testing), and if my ref_track patches will be accepted then they can go via net-dev tree and intel patches will wait till update of drm-tip.
Is this scenario OK?


Regards
Andrzej



-       stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC);
+       stack_handle = stack_depot_save(entries, nr_entries,
+                                       GFP_NOWAIT | __GFP_NOWARN);
This is fine.

         spin_lock_irqsave(&dir->lock, flags);
         if (tracker->dead) {
--
2.25.1





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux