Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
Hi Oleg, On 04/10/2018 04:36 PM, Oleg Nesterov wrote: > Hi Ravi, > > On 04/10, Ravi Bangoria wrote: >>> and what if __mmu_notifier_register() fails simply because signal_pending() >>> == T? >>> see mm_take_all_locks(). >>> >>> at first glance this all look suspicious and sub-optimal, >> Yes. I should have added checks for failure cases. >> Will fix them in v3. > And what can you do if it fails? Nothing except report the problem. But > signal_pending() is not the unlikely or error condition, it should not > cause the tracing errors. ... > Plus mm_take_all_locks() is very heavy... BTW, uprobe_mmap_callback() is > called unconditionally. Whatever it does, can we at least move it after > the no_uprobe_events() check? Can't we also check MMF_HAS_UPROBES? Sure, I'll move it after these conditions. > Either way, I do not feel that mmu_notifier is the right tool... Did you > consider the uprobe_clear_state() hook we already have? Ah! This is really a good idea. We don't need mmu_notifier then. Thanks for suggestion, Ravi -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
Hi Ravi, On 04/10, Ravi Bangoria wrote: > > > and what if __mmu_notifier_register() fails simply because signal_pending() > > == T? > > see mm_take_all_locks(). > > > > at first glance this all look suspicious and sub-optimal, > > Yes. I should have added checks for failure cases. > Will fix them in v3. And what can you do if it fails? Nothing except report the problem. But signal_pending() is not the unlikely or error condition, it should not cause the tracing errors. Plus mm_take_all_locks() is very heavy... BTW, uprobe_mmap_callback() is called unconditionally. Whatever it does, can we at least move it after the no_uprobe_events() check? Can't we also check MMF_HAS_UPROBES? Either way, I do not feel that mmu_notifier is the right tool... Did you consider the uprobe_clear_state() hook we already have? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
Hi Oleg,
On 04/09/2018 06:59 PM, Oleg Nesterov wrote:
> On 04/04, Ravi Bangoria wrote:
>> +static void sdt_add_mm_list(struct trace_uprobe *tu, struct mm_struct *mm)
>> +{
>> +struct mmu_notifier *mn;
>> +struct sdt_mm_list *sml = kzalloc(sizeof(*sml), GFP_KERNEL);
>> +
>> +if (!sml)
>> +return;
>> +sml->mm = mm;
>> +list_add(&(sml->list), &(tu->sml.list));
>> +
>> +/* Register mmu_notifier for this mm. */
>> +mn = kzalloc(sizeof(*mn), GFP_KERNEL);
>> +if (!mn)
>> +return;
>> +
>> +mn->ops = _mmu_notifier_ops;
>> +__mmu_notifier_register(mn, mm);
>> +}
> and what if __mmu_notifier_register() fails simply because signal_pending()
> == T?
> see mm_take_all_locks().
>
> at first glance this all look suspicious and sub-optimal,
Yes. I should have added checks for failure cases.
Will fix them in v3.
Thanks for the review,
Ravi
> but let me repeat that
> I didn't read this version yet.
>
> Oleg.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
On 04/09/2018 07:02 PM, Ravi Bangoria wrote: > Hi Oleg, > > On 04/09/2018 06:47 PM, Oleg Nesterov wrote: >> I didn't read this version yet, just one question... >> >> So now it depends on CONFIG_MMU_NOTIFIER, yes? I do not see any changes in >> Kconfig >> files, this doesn't look right... > Yes, you are write. s/write/right. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
Hi Oleg,
On 04/09/2018 06:47 PM, Oleg Nesterov wrote:
> On 04/04, Ravi Bangoria wrote:
>> +static void sdt_add_mm_list(struct trace_uprobe *tu, struct mm_struct *mm)
>> +{
>> +struct mmu_notifier *mn;
>> +struct sdt_mm_list *sml = kzalloc(sizeof(*sml), GFP_KERNEL);
>> +
>> +if (!sml)
>> +return;
>> +sml->mm = mm;
>> +list_add(&(sml->list), &(tu->sml.list));
>> +
>> +/* Register mmu_notifier for this mm. */
>> +mn = kzalloc(sizeof(*mn), GFP_KERNEL);
>> +if (!mn)
>> +return;
>> +
>> +mn->ops = _mmu_notifier_ops;
>> +__mmu_notifier_register(mn, mm);
>> +}
> I didn't read this version yet, just one question...
>
> So now it depends on CONFIG_MMU_NOTIFIER, yes? I do not see any changes in
> Kconfig
> files, this doesn't look right...
Yes, you are write. I'll make CONFIG_UPROBE_EVENTS dependent on
CONFIG_MMU_NOTIFIER.
Thanks,
Ravi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
On 04/04, Ravi Bangoria wrote:
>
> +static void sdt_add_mm_list(struct trace_uprobe *tu, struct mm_struct *mm)
> +{
> + struct mmu_notifier *mn;
> + struct sdt_mm_list *sml = kzalloc(sizeof(*sml), GFP_KERNEL);
> +
> + if (!sml)
> + return;
> + sml->mm = mm;
> + list_add(&(sml->list), &(tu->sml.list));
> +
> + /* Register mmu_notifier for this mm. */
> + mn = kzalloc(sizeof(*mn), GFP_KERNEL);
> + if (!mn)
> + return;
> +
> + mn->ops = _mmu_notifier_ops;
> + __mmu_notifier_register(mn, mm);
> +}
and what if __mmu_notifier_register() fails simply because signal_pending() ==
T?
see mm_take_all_locks().
at first glance this all look suspicious and sub-optimal, but let me repeat that
I didn't read this version yet.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
On 04/04, Ravi Bangoria wrote:
>
> +static void sdt_add_mm_list(struct trace_uprobe *tu, struct mm_struct *mm)
> +{
> + struct mmu_notifier *mn;
> + struct sdt_mm_list *sml = kzalloc(sizeof(*sml), GFP_KERNEL);
> +
> + if (!sml)
> + return;
> + sml->mm = mm;
> + list_add(&(sml->list), &(tu->sml.list));
> +
> + /* Register mmu_notifier for this mm. */
> + mn = kzalloc(sizeof(*mn), GFP_KERNEL);
> + if (!mn)
> + return;
> +
> + mn->ops = _mmu_notifier_ops;
> + __mmu_notifier_register(mn, mm);
> +}
I didn't read this version yet, just one question...
So now it depends on CONFIG_MMU_NOTIFIER, yes? I do not see any changes in
Kconfig
files, this doesn't look right...
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/9] trace_uprobe/sdt: Fix multiple update of same reference counter
Hi Ravi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.16 next-20180403]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Ravi-Bangoria/trace_uprobe-Support-SDT-markers-having-reference-count-semaphore/20180404-201900
config: i386-randconfig-a0-201813 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel//trace/trace_uprobe.c:947:21: error: variable 'sdt_mmu_notifier_ops'
has initializer but incomplete type
static const struct mmu_notifier_ops sdt_mmu_notifier_ops = {
^
>> kernel//trace/trace_uprobe.c:948:2: error: unknown field 'release' specified
>> in initializer
.release = sdt_mm_release,
^
kernel//trace/trace_uprobe.c:948:2: warning: excess elements in struct
initializer
kernel//trace/trace_uprobe.c:948:2: warning: (near initialization for
'sdt_mmu_notifier_ops')
kernel//trace/trace_uprobe.c: In function 'sdt_add_mm_list':
>> kernel//trace/trace_uprobe.c:962:22: error: dereferencing pointer to
>> incomplete type
mn = kzalloc(sizeof(*mn), GFP_KERNEL);
^
kernel//trace/trace_uprobe.c:966:4: error: dereferencing pointer to
incomplete type
mn->ops = _mmu_notifier_ops;
^
>> kernel//trace/trace_uprobe.c:967:2: error: implicit declaration of function
>> '__mmu_notifier_register' [-Werror=implicit-function-declaration]
__mmu_notifier_register(mn, mm);
^
cc1: some warnings being treated as errors
vim +/__mmu_notifier_register +967 kernel//trace/trace_uprobe.c
946
> 947 static const struct mmu_notifier_ops sdt_mmu_notifier_ops = {
> 948 .release = sdt_mm_release,
949 };
950
951 static void sdt_add_mm_list(struct trace_uprobe *tu, struct mm_struct
*mm)
952 {
953 struct mmu_notifier *mn;
954 struct sdt_mm_list *sml = kzalloc(sizeof(*sml), GFP_KERNEL);
955
956 if (!sml)
957 return;
958 sml->mm = mm;
959 list_add(&(sml->list), &(tu->sml.list));
960
961 /* Register mmu_notifier for this mm. */
> 962 mn = kzalloc(sizeof(*mn), GFP_KERNEL);
963 if (!mn)
964 return;
965
966 mn->ops = _mmu_notifier_ops;
> 967 __mmu_notifier_register(mn, mm);
968 }
969
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
