| Message ID | 20220217140441.1218045-6-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: use ref_tracker library for tracking wakerefs | expand |
On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@intel.com> 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") > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > --- > 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. > > 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; > + gfp_t gfp_mask; > unsigned long flags; > Then leave all this code as is ? I find current code more readable. > - 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 > - 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 >
On 17.02.2022 16:13, Eric Dumazet wrote: > On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@intel.com> 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@intel.com> >> --- >> 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 >>
On Fri, Feb 18, 2022 at 2:28 AM Andrzej Hajda <andrzej.hajda@intel.com> wrote: > > 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? One possibility would be to split your patch series in two, to merge the ref_tracker changes directly in net-next asap. But I have no strong opinion, maybe Jakub/David have some guidance.
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); 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; + gfp_t gfp_mask; unsigned long flags; - 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); - stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); + stack_handle = stack_depot_save(entries, nr_entries, + GFP_NOWAIT | __GFP_NOWARN); spin_lock_irqsave(&dir->lock, flags); if (tracker->dead) {
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. Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> --- lib/ref_tracker.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)