From: Andrzej Hajda <andrzej.hajda@intel.com>
To: Eric Dumazet <edumazet@google.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
<intel-gfx@lists.freedesktop.org>,
<dri-devel@lists.freedesktop.org>,
netdev <netdev@vger.kernel.org>,
"Jani Nikula" <jani.nikula@linux.intel.com>,
Daniel Vetter <daniel@ffwll.ch>,
"Lucas De Marchi" <lucas.demarchi@intel.com>,
Chris Wilson <chris.p.wilson@intel.com>,
Dmitry Vyukov <dvyukov@google.com>,
"Jakub Kicinski" <kuba@kernel.org>
Subject: Re: [PATCH 5/9] lib/ref_tracker: improve allocation flags
Date: Fri, 18 Feb 2022 11:28:37 +0100 [thread overview]
Message-ID: <8f6fb098-d0d9-910e-7d42-9fbc541f9891@intel.com> (raw)
In-Reply-To: <CANn89i+nCZ6LV_1E2OnJ4qWE0XkO2FGW+A6_tkmQpdxiiEh=LQ@mail.gmail.com>
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
>>
next prev parent reply other threads:[~2022-02-18 10:28 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-17 14:04 [PATCH 0/9] drm/i915: use ref_tracker library for tracking wakerefs Andrzej Hajda
2022-02-17 14:04 ` [PATCH 1/9] lib/ref_tracker: add unlocked leak print helper Andrzej Hajda
2022-02-17 14:04 ` [PATCH 2/9] lib/ref_tracker: compact stacktraces before printing Andrzej Hajda
2022-02-17 15:23 ` Eric Dumazet
2022-02-17 15:25 ` Eric Dumazet
2022-02-18 10:54 ` Andrzej Hajda
2022-02-18 13:01 ` Eric Dumazet
2022-02-17 14:04 ` [PATCH 3/9] lib/ref_tracker: __ref_tracker_dir_print improve printing Andrzej Hajda
2022-02-17 15:38 ` Eric Dumazet
2022-02-18 10:11 ` Andrzej Hajda
2022-02-17 14:04 ` [PATCH 4/9] lib/ref_tracker: add printing to memory buffer Andrzej Hajda
2022-02-17 14:04 ` [PATCH 5/9] lib/ref_tracker: improve allocation flags Andrzej Hajda
2022-02-17 15:13 ` Eric Dumazet
2022-02-18 10:28 ` Andrzej Hajda [this message]
2022-02-18 13:05 ` Eric Dumazet
2022-02-17 14:04 ` [PATCH 6/9] drm/i915: Separate wakeref tracking Andrzej Hajda
2022-02-17 14:48 ` [Intel-gfx] " Ville Syrjälä
2022-02-18 10:32 ` Andrzej Hajda
2022-02-17 14:04 ` [PATCH 7/9] drm/i915: Track leaked gt->wakerefs Andrzej Hajda
2022-02-17 14:04 ` [PATCH 8/9] drm/i915: Correct type of wakeref variable Andrzej Hajda
2022-02-17 14:04 ` [PATCH 9/9] drm/i915: replace Intel internal tracker with kernel core ref_tracker Andrzej Hajda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8f6fb098-d0d9-910e-7d42-9fbc541f9891@intel.com \
--to=andrzej.hajda@intel.com \
--cc=chris.p.wilson@intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=dvyukov@google.com \
--cc=edumazet@google.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lucas.demarchi@intel.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).