All of lore.kernel.org
 help / color / mirror / Atom feed
From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
To: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: oleg@redhat.com, rostedt@goodmis.org, mhiramat@kernel.org,
	peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
	alexander.shishkin@linux.intel.com, jolsa@redhat.com,
	namhyung@kernel.org, linux-kernel@vger.kernel.org,
	corbet@lwn.net, linux-doc@vger.kernel.org,
	ananth@linux.vnet.ibm.com, alexis.berlemont@gmail.com,
	naveen.n.rao@linux.vnet.ibm.com,
	linux-arm-kernel@lists.infradead.org, linux-mips@linux-mips.org,
	linux@armlinux.org.uk, ralf@linux-mips.org, paul.burton@mips.com
Subject: Re: [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore)
Date: Mon, 2 Jul 2018 09:01:58 -0700	[thread overview]
Message-ID: <20180702160158.GD65296@linux.vnet.ibm.com> (raw)
In-Reply-To: <20180628052209.13056-7-ravi.bangoria@linux.ibm.com>

> Implement the reference counter logic in core uprobe. User will be
> able to use it from trace_uprobe as well as from kernel module. New
> trace_uprobe definition with reference counter will now be:
> 
>     <path>:<offset>[(ref_ctr_offset)]
> 
> where ref_ctr_offset is an optional field. For kernel module, new
> variant of uprobe_register() has been introduced:
> 
>     uprobe_register_refctr(inode, offset, ref_ctr_offset, consumer)
> 

Sorry for bringing this again, but I would actually think the ref_ctr is
a consumer property. i.e the ref_ctr_offset should be part of
uprobe_consumer.

The advantages of doing that would be
1. Dont need to expose uprobe structure and just update our
uprobe_consumer which is already an exported structure.
- Exporting uprobe structure would expose some of our internal
  implementation details, basically reduce the freedom of changing stuff
  internally.
- we came up with uprobe_arch for the parts that we wanted to expose
  to archs. exposing uprobe and uprobe_arch looks weird.

2. ref_ctr_offset is necessarily a consumer property, its not a uprobe
property at all.

3. We dont need to change/add new uprobe_register functions.

The way I look at it is.

Based on the ref_ctr_offset field in consumer, we update_ref_ctr()
around install_breakpoint/remove_breakpoint.

> +static int delayed_uprobe_add(struct uprobe *uprobe, struct mm_struct *mm)
> +{
> +	struct delayed_uprobe *du;
> +
> +	if (delayed_uprobe_check(uprobe, mm))
> +		return 0;
> +
> +	du  = kzalloc(sizeof(*du), GFP_KERNEL);
> +	if (!du)
> +		return -ENOMEM;
> +
> +	du->uprobe = uprobe;
> +	du->mm = mm;
> +	list_add(&du->list, &delayed_uprobe_list);
> +	return 0;
> +}
> +

If I understood the delayed_uprobe stuff, its when we could insert a
breakpoint but the vma that has the ref_ctr_offset is not loaded. Is
that correct?

> 
> -- 
> 2.14.4
> 


WARNING: multiple messages have this Message-ID (diff)
From: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
To: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: oleg@redhat.com, rostedt@goodmis.org, mhiramat@kernel.org,
	peterz@infradead.org, mingo@redhat.com, acme@kernel.org,
	alexander.shishkin@linux.intel.com, jolsa@redhat.com,
	namhyung@kernel.org, linux-kernel@vger.kernel.org,
	corbet@lwn.net, linux-doc@vger.kernel.org,
	ananth@linux.vnet.ibm.com, alexis.berlemont@gmail.com,
	naveen.n.rao@linux.vnet.ibm.com,
	linux-arm-kernel@lists.infradead.org, linux-mips@linux-mips.org,
	linux@armlinux.org.uk, ralf@linux-mips.org, paul.burton@mips.com
Subject: Re: [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore)
Date: Mon, 2 Jul 2018 09:01:58 -0700	[thread overview]
Message-ID: <20180702160158.GD65296@linux.vnet.ibm.com> (raw)
In-Reply-To: <20180628052209.13056-7-ravi.bangoria@linux.ibm.com>

> Implement the reference counter logic in core uprobe. User will be
> able to use it from trace_uprobe as well as from kernel module. New
> trace_uprobe definition with reference counter will now be:
> 
>     <path>:<offset>[(ref_ctr_offset)]
> 
> where ref_ctr_offset is an optional field. For kernel module, new
> variant of uprobe_register() has been introduced:
> 
>     uprobe_register_refctr(inode, offset, ref_ctr_offset, consumer)
> 

Sorry for bringing this again, but I would actually think the ref_ctr is
a consumer property. i.e the ref_ctr_offset should be part of
uprobe_consumer.

The advantages of doing that would be
1. Dont need to expose uprobe structure and just update our
uprobe_consumer which is already an exported structure.
- Exporting uprobe structure would expose some of our internal
  implementation details, basically reduce the freedom of changing stuff
  internally.
- we came up with uprobe_arch for the parts that we wanted to expose
  to archs. exposing uprobe and uprobe_arch looks weird.

2. ref_ctr_offset is necessarily a consumer property, its not a uprobe
property at all.

3. We dont need to change/add new uprobe_register functions.

The way I look at it is.

Based on the ref_ctr_offset field in consumer, we update_ref_ctr()
around install_breakpoint/remove_breakpoint.

> +static int delayed_uprobe_add(struct uprobe *uprobe, struct mm_struct *mm)
> +{
> +	struct delayed_uprobe *du;
> +
> +	if (delayed_uprobe_check(uprobe, mm))
> +		return 0;
> +
> +	du  = kzalloc(sizeof(*du), GFP_KERNEL);
> +	if (!du)
> +		return -ENOMEM;
> +
> +	du->uprobe = uprobe;
> +	du->mm = mm;
> +	list_add(&du->list, &delayed_uprobe_list);
> +	return 0;
> +}
> +

If I understood the delayed_uprobe stuff, its when we could insert a
breakpoint but the vma that has the ref_ctr_offset is not loaded. Is
that correct?

> 
> -- 
> 2.14.4
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID (diff)
From: srikar@linux.vnet.ibm.com (Srikar Dronamraju)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore)
Date: Mon, 2 Jul 2018 09:01:58 -0700	[thread overview]
Message-ID: <20180702160158.GD65296@linux.vnet.ibm.com> (raw)
In-Reply-To: <20180628052209.13056-7-ravi.bangoria@linux.ibm.com>

> Implement the reference counter logic in core uprobe. User will be
> able to use it from trace_uprobe as well as from kernel module. New
> trace_uprobe definition with reference counter will now be:
> 
>     <path>:<offset>[(ref_ctr_offset)]
> 
> where ref_ctr_offset is an optional field. For kernel module, new
> variant of uprobe_register() has been introduced:
> 
>     uprobe_register_refctr(inode, offset, ref_ctr_offset, consumer)
> 

Sorry for bringing this again, but I would actually think the ref_ctr is
a consumer property. i.e the ref_ctr_offset should be part of
uprobe_consumer.

The advantages of doing that would be
1. Dont need to expose uprobe structure and just update our
uprobe_consumer which is already an exported structure.
- Exporting uprobe structure would expose some of our internal
  implementation details, basically reduce the freedom of changing stuff
  internally.
- we came up with uprobe_arch for the parts that we wanted to expose
  to archs. exposing uprobe and uprobe_arch looks weird.

2. ref_ctr_offset is necessarily a consumer property, its not a uprobe
property at all.

3. We dont need to change/add new uprobe_register functions.

The way I look at it is.

Based on the ref_ctr_offset field in consumer, we update_ref_ctr()
around install_breakpoint/remove_breakpoint.

> +static int delayed_uprobe_add(struct uprobe *uprobe, struct mm_struct *mm)
> +{
> +	struct delayed_uprobe *du;
> +
> +	if (delayed_uprobe_check(uprobe, mm))
> +		return 0;
> +
> +	du  = kzalloc(sizeof(*du), GFP_KERNEL);
> +	if (!du)
> +		return -ENOMEM;
> +
> +	du->uprobe = uprobe;
> +	du->mm = mm;
> +	list_add(&du->list, &delayed_uprobe_list);
> +	return 0;
> +}
> +

If I understood the delayed_uprobe stuff, its when we could insert a
breakpoint but the vma that has the ref_ctr_offset is not loaded. Is
that correct?

> 
> -- 
> 2.14.4
> 

  parent reply	other threads:[~2018-07-02 16:02 UTC|newest]

Thread overview: 152+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-28  5:21 [PATCH v5 00/10] Uprobes: Support SDT markers having reference count (semaphore) Ravi Bangoria
2018-06-28  5:21 ` Ravi Bangoria
2018-06-28  5:21 ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 01/10] Uprobes: Move uprobe structure to uprobe.h Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 02/10] Uprobes: Simplify uprobe_register() body Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 03/10] Uprobe: Change set_swbp definition Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 04/10] Uprobe: Change set_orig_insn definition Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 05/10] Uprobe: Change uprobe_write_opcode definition Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore) Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28 19:51   ` Oleg Nesterov
2018-06-28 19:51     ` Oleg Nesterov
2018-06-28 19:51     ` Oleg Nesterov
2018-06-29  3:23     ` Ravi Bangoria
2018-06-29  3:23       ` Ravi Bangoria
2018-06-29  3:23       ` Ravi Bangoria
2018-07-01 21:09   ` Oleg Nesterov
2018-07-01 21:09     ` Oleg Nesterov
2018-07-01 21:09     ` Oleg Nesterov
2018-07-02  5:16     ` Ravi Bangoria
2018-07-02  5:16       ` Ravi Bangoria
2018-07-02  5:16       ` Ravi Bangoria
2018-07-02 18:01       ` Oleg Nesterov
2018-07-02 18:01         ` Oleg Nesterov
2018-07-02 18:01         ` Oleg Nesterov
2018-07-03  5:30         ` Ravi Bangoria
2018-07-03  5:30           ` Ravi Bangoria
2018-07-03  5:30           ` Ravi Bangoria
2018-07-03  6:16           ` Srikar Dronamraju
2018-07-03  6:16             ` Srikar Dronamraju
2018-07-03  6:16             ` Srikar Dronamraju
2018-07-03  7:43             ` Ravi Bangoria
2018-07-03  7:43               ` Ravi Bangoria
2018-07-03  7:43               ` Ravi Bangoria
2018-07-04  9:16               ` Srikar Dronamraju
2018-07-04  9:16                 ` Srikar Dronamraju
2018-07-04  9:16                 ` Srikar Dronamraju
2018-07-04  9:24                 ` Ravi Bangoria
2018-07-04  9:24                   ` Ravi Bangoria
2018-07-04  9:24                   ` Ravi Bangoria
2018-07-03  8:11           ` Ravi Bangoria
2018-07-03  8:11             ` Ravi Bangoria
2018-07-03  8:11             ` Ravi Bangoria
2018-07-03 16:36           ` Oleg Nesterov
2018-07-03 16:36             ` Oleg Nesterov
2018-07-03 16:36             ` Oleg Nesterov
2018-07-03 17:25             ` Oleg Nesterov
2018-07-03 17:25               ` Oleg Nesterov
2018-07-03 17:25               ` Oleg Nesterov
2018-07-04  4:53               ` Ravi Bangoria
2018-07-04  4:53                 ` Ravi Bangoria
2018-07-04  4:53                 ` Ravi Bangoria
2018-07-10 15:25                 ` Oleg Nesterov
2018-07-10 15:25                   ` Oleg Nesterov
2018-07-10 15:25                   ` Oleg Nesterov
2018-07-11  8:44                   ` Ravi Bangoria
2018-07-11  8:44                     ` Ravi Bangoria
2018-07-11  8:44                     ` Ravi Bangoria
2018-07-11  9:52                     ` Ravi Bangoria
2018-07-11  9:52                       ` Ravi Bangoria
2018-07-11  9:52                       ` Ravi Bangoria
2018-07-12 14:58                     ` Oleg Nesterov
2018-07-12 14:58                       ` Oleg Nesterov
2018-07-12 14:58                       ` Oleg Nesterov
2018-07-12 19:53                       ` Song Liu
2018-07-12 19:53                         ` Song Liu
2018-07-12 19:53                         ` Song Liu
2018-07-12 19:53                         ` Song Liu
2018-07-13  7:55                         ` Ravi Bangoria
2018-07-13  7:55                           ` Ravi Bangoria
2018-07-13  7:55                           ` Ravi Bangoria
2018-07-13 23:50                           ` Song Liu
2018-07-13 23:50                             ` Song Liu
2018-07-13 23:50                             ` Song Liu
2018-07-13 23:50                             ` Song Liu
2018-07-16  8:20                             ` Ravi Bangoria
2018-07-16  8:20                               ` Ravi Bangoria
2018-07-16  8:20                               ` Ravi Bangoria
2018-07-16  8:51                             ` Ravi Bangoria
2018-07-16  8:51                               ` Ravi Bangoria
2018-07-16  8:51                               ` Ravi Bangoria
2018-07-13  5:39                       ` Ravi Bangoria
2018-07-13  5:39                         ` Ravi Bangoria
2018-07-13  5:39                         ` Ravi Bangoria
2018-07-04  4:49             ` Ravi Bangoria
2018-07-04  4:49               ` Ravi Bangoria
2018-07-04  4:49               ` Ravi Bangoria
2018-07-03 17:12           ` Oleg Nesterov
2018-07-03 17:12             ` Oleg Nesterov
2018-07-03 17:12             ` Oleg Nesterov
2018-07-03 18:23             ` Oleg Nesterov
2018-07-03 18:23               ` Oleg Nesterov
2018-07-03 18:23               ` Oleg Nesterov
2018-07-04  5:25               ` Ravi Bangoria
2018-07-04  5:25                 ` Ravi Bangoria
2018-07-04  5:25                 ` Ravi Bangoria
2018-07-02 16:01   ` Srikar Dronamraju [this message]
2018-07-02 16:01     ` Srikar Dronamraju
2018-07-02 16:01     ` Srikar Dronamraju
2018-07-02 18:05     ` Oleg Nesterov
2018-07-02 18:05       ` Oleg Nesterov
2018-07-02 18:05       ` Oleg Nesterov
2018-07-03  6:29     ` Ravi Bangoria
2018-07-03  6:29       ` Ravi Bangoria
2018-07-03  6:29       ` Ravi Bangoria
2018-07-03 19:26       ` Oleg Nesterov
2018-07-03 19:26         ` Oleg Nesterov
2018-07-03 19:26         ` Oleg Nesterov
2018-07-04  5:26         ` Ravi Bangoria
2018-07-04  5:26           ` Ravi Bangoria
2018-07-04  5:26           ` Ravi Bangoria
2018-07-04  6:07     ` Ravi Bangoria
2018-07-04  6:07       ` Ravi Bangoria
2018-07-04  6:07       ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 07/10] trace_uprobe/sdt: Prevent multiple reference counter for same uprobe Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 08/10] Uprobes/sdt: " Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 09/10] Uprobes/sdt: Document about reference counter Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-07-02 14:54   ` Srikar Dronamraju
2018-07-02 14:54     ` Srikar Dronamraju
2018-07-02 14:54     ` Srikar Dronamraju
2018-07-03  7:50     ` Ravi Bangoria
2018-07-03  7:50       ` Ravi Bangoria
2018-07-03  7:50       ` Ravi Bangoria
2018-06-28  5:22 ` [PATCH v5 10/10] perf probe: Support SDT markers having reference counter (semaphore) Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-06-28  5:22   ` Ravi Bangoria
2018-07-02 14:45   ` Srikar Dronamraju
2018-07-02 14:45     ` Srikar Dronamraju
2018-07-02 14:45     ` Srikar Dronamraju
2018-07-02 14:57   ` Srikar Dronamraju
2018-07-02 14:57     ` Srikar Dronamraju
2018-07-02 14:57     ` Srikar Dronamraju
2018-07-03  8:00     ` Ravi Bangoria
2018-07-03  8:00       ` Ravi Bangoria
2018-07-03  8:00       ` Ravi Bangoria

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=20180702160158.GD65296@linux.vnet.ibm.com \
    --to=srikar@linux.vnet.ibm.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=alexis.berlemont@gmail.com \
    --cc=ananth@linux.vnet.ibm.com \
    --cc=corbet@lwn.net \
    --cc=jolsa@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@linux-mips.org \
    --cc=linux@armlinux.org.uk \
    --cc=mhiramat@kernel.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=naveen.n.rao@linux.vnet.ibm.com \
    --cc=oleg@redhat.com \
    --cc=paul.burton@mips.com \
    --cc=peterz@infradead.org \
    --cc=ralf@linux-mips.org \
    --cc=ravi.bangoria@linux.ibm.com \
    --cc=rostedt@goodmis.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.