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 <[email protected]>
---
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);
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) {
--
2.25.1
On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]> 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.