From: Yonghong Song <yhs@fb•com>
To: Jonathan Lemon <jonathan.lemon@gmail•com>
Cc: <netdev@vger•kernel.org>, <ast@kernel•org>,
<daniel@iogearbox•net>, <kernel-team@fb•com>
Subject: Re: [PATCH v2 bpf-next] bpf: increment and use correct thread iterator
Date: Sun, 13 Dec 2020 23:01:49 -0800 [thread overview]
Message-ID: <c1d02027-80d4-2d2c-a254-ccd4c7fa2239@fb.com> (raw)
In-Reply-To: <20201211163017.3fjxnuickl2m523m@bsd-mbp.dhcp.thefacebook.com>
On 12/11/20 8:30 AM, Jonathan Lemon wrote:
> On Wed, Dec 09, 2020 at 11:02:54AM -0800, Yonghong Song wrote:
>>
>>
>> Maybe you can post v3 of the patch with the above information in the
>> commit description so people can better understand what the problem
>> you are trying to solve here?
>>
>> Also, could you also send to bpf@vger•kernel.org?
>
> Sure, I can do that.
>
>>>>> If unable to obtain the file structure for the current task,
>>>>> proceed to the next task number after the one returned from
>>>>> task_seq_get_next(), instead of the next task number from the
>>>>> original iterator.
>>>> This seems a correct change. The current code should still work
>>>> but it may do some redundant/unnecessary work in kernel.
>>>> This only happens when a task does not have any file,
>>>> no sure whether this is the culprit for the problem this
>>>> patch tries to address.
>>>>
>>>>>
>>>>> Use thread_group_leader() instead of comparing tgid vs pid, which
>>>>> might may be racy.
>>>>
>>>> I see
>>>>
>>>> static inline bool thread_group_leader(struct task_struct *p)
>>>> {
>>>> return p->exit_signal >= 0;
>>>> }
>>>>
>>>> I am not sure whether thread_group_leader(task) is equivalent
>>>> to task->tgid == task->pid or not. Any documentation or explanation?
>>>>
>>>> Could you explain why task->tgid != task->pid in the original
>>>> code could be racy?
>>>
>>> My understanding is that anything which uses pid_t for comparision
>>> in the kernel is incorrect. Looking at de_thread(), there is a
>>> section which swaps the pid and tids around, but doesn't seem to
>>> change tgid directly.
>>>
>>> There's also this comment in linux/pid.h:
>>> /*
>>> * Both old and new leaders may be attached to
>>> * the same pid in the middle of de_thread().
>>> */
>>>
>>> So the safest thing to do is use the explicit thread_group_leader()
>>> macro rather than trying to open code things.
>>
>> I did some limited experiments and did not trigger a case where
>> task->tgid != task->pid not agreeing with !thread_group_leader().
>> Will need more tests in the environment to reproduce the warning
>> to confirm whether this is the culprit or not.
>
> Perhaps, but on the other hand, the splats disappear with this
> patch, so it's doing something right. If your debug code hasn't
> detected any cases where thread_group_leader() isn't making a
> difference, then there shouldn't be any objections in making the
> replacement, right? It does make the code easier to understand
> and matches the rest of the kernel.
Agree. Let me do a little more experiments to double check we
did not miss anything with this particular change and will
report back later.
prev parent reply other threads:[~2020-12-14 7:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-04 3:43 [PATCH v2 bpf-next] bpf: increment and use correct thread iterator Jonathan Lemon
2020-12-04 8:01 ` Yonghong Song
2020-12-04 17:14 ` Jonathan Lemon
2020-12-04 18:54 ` Yonghong Song
2020-12-09 19:02 ` Yonghong Song
2020-12-11 16:30 ` Jonathan Lemon
2020-12-14 7:01 ` Yonghong Song [this message]
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=c1d02027-80d4-2d2c-a254-ccd4c7fa2239@fb.com \
--to=yhs@fb$(echo .)com \
--cc=ast@kernel$(echo .)org \
--cc=daniel@iogearbox$(echo .)net \
--cc=jonathan.lemon@gmail$(echo .)com \
--cc=kernel-team@fb$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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