Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On 04.06.13 11:35:22, Borislav Petkov wrote:
> On Tue, Jun 04, 2013 at 11:19:21AM +0200, Jiri Olsa wrote:
> > 'cpu' did not pass ay check here..
>
> Oh, I see.
>
> You mean something like
>
> if (cpu < NR_CPUS && cpu_online(cpu))
In perf_event_alloc() there is:
if ((unsigned)cpu >= nr_cpu_ids) {
if (!task || cpu != -1)
return ERR_PTR(-EINVAL);
}
So this should be sufficient:
if ((unsigned)cpu >= nr_cpu_ids)
...
-Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On 04.06.13 11:35:22, Borislav Petkov wrote:
On Tue, Jun 04, 2013 at 11:19:21AM +0200, Jiri Olsa wrote:
'cpu' did not pass ay check here..
Oh, I see.
You mean something like
if (cpu NR_CPUS cpu_online(cpu))
In perf_event_alloc() there is:
if ((unsigned)cpu = nr_cpu_ids) {
if (!task || cpu != -1)
return ERR_PTR(-EINVAL);
}
So this should be sufficient:
if ((unsigned)cpu = nr_cpu_ids)
...
-Robert
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Tue, Jun 04, 2013 at 10:20:42AM +0200, Borislav Petkov wrote:
> On Mon, Jun 03, 2013 at 03:49:25PM +0200, Jiri Olsa wrote:
> > maybe check for valid cpu, since perf_get_persistent_event_fd is
> > called directly from syscall allowing anything in cpu
>
> That should be fine - we're traversing a per-cpu list of events there.
hum, can't see it:
perf SYSCALL:
...
return perf_get_persistent_event_fd(cpu, );
...
int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
struct perf_event *event;
int event_fd = -ENODEV;
mutex_lock(_cpu(pers_events_lock, cpu));
...
'cpu' did not pass ay check here..
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Tue, Jun 04, 2013 at 11:19:21AM +0200, Jiri Olsa wrote:
> hum, can't see it:
>
> perf SYSCALL:
> ...
> return perf_get_persistent_event_fd(cpu, );
>
> ...
> int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
> {
> struct pers_event_desc *desc;
> struct perf_event *event;
> int event_fd = -ENODEV;
>
> mutex_lock(_cpu(pers_events_lock, cpu));
> ...
>
> 'cpu' did not pass ay check here..
Oh, I see.
You mean something like
if (cpu < NR_CPUS && cpu_online(cpu))
?
I guess that should cover your concerns...
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Mon, Jun 03, 2013 at 03:49:25PM +0200, Jiri Olsa wrote: > maybe check for valid cpu, since perf_get_persistent_event_fd is > called directly from syscall allowing anything in cpu That should be fine - we're traversing a per-cpu list of events there. > (unrelated to this patch, but I couldn't find the original > patch that adds perf_get_persistent_event_fd) Try this: http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=persistent5.1 :-) -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Mon, Jun 03, 2013 at 03:49:25PM +0200, Jiri Olsa wrote: maybe check for valid cpu, since perf_get_persistent_event_fd is called directly from syscall allowing anything in cpu That should be fine - we're traversing a per-cpu list of events there. (unrelated to this patch, but I couldn't find the original patch that adds perf_get_persistent_event_fd) Try this: http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=persistent5.1 :-) -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Tue, Jun 04, 2013 at 11:19:21AM +0200, Jiri Olsa wrote:
hum, can't see it:
perf SYSCALL:
...
return perf_get_persistent_event_fd(cpu, attr);
...
int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
struct perf_event *event;
int event_fd = -ENODEV;
mutex_lock(per_cpu(pers_events_lock, cpu));
...
'cpu' did not pass ay check here..
Oh, I see.
You mean something like
if (cpu NR_CPUS cpu_online(cpu))
?
I guess that should cover your concerns...
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Tue, Jun 04, 2013 at 10:20:42AM +0200, Borislav Petkov wrote:
On Mon, Jun 03, 2013 at 03:49:25PM +0200, Jiri Olsa wrote:
maybe check for valid cpu, since perf_get_persistent_event_fd is
called directly from syscall allowing anything in cpu
That should be fine - we're traversing a per-cpu list of events there.
hum, can't see it:
perf SYSCALL:
...
return perf_get_persistent_event_fd(cpu, attr);
...
int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
struct perf_event *event;
int event_fd = -ENODEV;
mutex_lock(per_cpu(pers_events_lock, cpu));
...
'cpu' did not pass ay check here..
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Fri, May 31, 2013 at 10:47:36AM +0200, Robert Richter wrote:
> From: Robert Richter
SNIP
> out:
> mutex_unlock(_cpu(pers_events_lock, cpu));
> }
> @@ -182,18 +195,31 @@ fail:
> int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
> {
> struct pers_event_desc *desc;
> + struct perf_event *event;
> int event_fd = -ENODEV;
>
> mutex_lock(_cpu(pers_events_lock, cpu));
maybe check for valid cpu, since perf_get_persistent_event_fd is
called directly from syscall allowing anything in cpu
(unrelated to this patch, but I couldn't find the original
patch that adds perf_get_persistent_event_fd)
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 16/16] perf, persistent: Allow multiple users for an event
On Fri, May 31, 2013 at 10:47:36AM +0200, Robert Richter wrote:
From: Robert Richter robert.rich...@calxeda.com
SNIP
out:
mutex_unlock(per_cpu(pers_events_lock, cpu));
}
@@ -182,18 +195,31 @@ fail:
int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
+ struct perf_event *event;
int event_fd = -ENODEV;
mutex_lock(per_cpu(pers_events_lock, cpu));
maybe check for valid cpu, since perf_get_persistent_event_fd is
called directly from syscall allowing anything in cpu
(unrelated to this patch, but I couldn't find the original
patch that adds perf_get_persistent_event_fd)
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/16] perf, persistent: Allow multiple users for an event
From: Robert Richter
Usually a fd close leads to the release of the event too. For
persistent events this is different as the events should be
permanently enabled in the system. Using reference counting to avoid
releasing an event during a fd close. This also allows it to have
multiple users (open file descriptors) for a single persistent event.
While at this, we don't need desc->fd any longer. The fd is attached
to a task and reference counting keeps the event. Removing desc->fd.
Signed-off-by: Robert Richter
---
kernel/events/persistent.c | 46 --
1 file changed, 36 insertions(+), 10 deletions(-)
diff --git a/kernel/events/persistent.c b/kernel/events/persistent.c
index a764144..4920702 100644
--- a/kernel/events/persistent.c
+++ b/kernel/events/persistent.c
@@ -11,7 +11,6 @@
struct pers_event_desc {
struct perf_event *event;
struct list_head plist;
- int fd;
};
struct pers_event {
@@ -88,6 +87,18 @@ out:
return event;
}
+static void detach_persistent_event(struct pers_event_desc *desc)
+{
+ list_del(>plist);
+ kfree(desc);
+}
+
+static void release_persistent_event(struct perf_event *event)
+{
+ perf_event_disable(event);
+ perf_event_release_kernel(event);
+}
+
static void del_persistent_event(int cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
@@ -100,12 +111,14 @@ static void del_persistent_event(int cpu, struct
perf_event_attr *attr)
goto out;
event = desc->event;
- list_del(>plist);
-
- perf_event_disable(event);
- perf_event_release_kernel(event);
- put_unused_fd(desc->fd);
- kfree(desc);
+ /*
+* We primarily want to remove desc from the list. If there
+* are no open files, the refcount is 0 and we need to release
+* the event too.
+*/
+ detach_persistent_event(desc);
+ if (atomic_long_dec_and_test(>refcount))
+ release_persistent_event(event);
out:
mutex_unlock(_cpu(pers_events_lock, cpu));
}
@@ -182,18 +195,31 @@ fail:
int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
+ struct perf_event *event;
int event_fd = -ENODEV;
mutex_lock(_cpu(pers_events_lock, cpu));
desc = get_persistent_event(cpu, attr);
- if (!desc)
+
+ /* Increment refcount to keep event on put_event() */
+ if (!desc || !atomic_long_inc_not_zero(>event->refcount))
goto out;
event_fd = anon_inode_getfd("[pers_event]", _fops,
desc->event, O_RDONLY);
- if (event_fd >= 0)
- desc->fd = event_fd;
+
+ if (event_fd < 0) {
+ event = desc->event;
+ if (WARN_ON(atomic_long_dec_and_test(>refcount))) {
+ /*
+* May not happen since decrementing refcount is
+* protected by pers_events_lock.
+*/
+ detach_persistent_event(desc);
+ release_persistent_event(event);
+ }
+ }
out:
mutex_unlock(_cpu(pers_events_lock, cpu));
--
1.8.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 16/16] perf, persistent: Allow multiple users for an event
From: Robert Richter robert.rich...@calxeda.com
Usually a fd close leads to the release of the event too. For
persistent events this is different as the events should be
permanently enabled in the system. Using reference counting to avoid
releasing an event during a fd close. This also allows it to have
multiple users (open file descriptors) for a single persistent event.
While at this, we don't need desc-fd any longer. The fd is attached
to a task and reference counting keeps the event. Removing desc-fd.
Signed-off-by: Robert Richter robert.rich...@calxeda.com
---
kernel/events/persistent.c | 46 --
1 file changed, 36 insertions(+), 10 deletions(-)
diff --git a/kernel/events/persistent.c b/kernel/events/persistent.c
index a764144..4920702 100644
--- a/kernel/events/persistent.c
+++ b/kernel/events/persistent.c
@@ -11,7 +11,6 @@
struct pers_event_desc {
struct perf_event *event;
struct list_head plist;
- int fd;
};
struct pers_event {
@@ -88,6 +87,18 @@ out:
return event;
}
+static void detach_persistent_event(struct pers_event_desc *desc)
+{
+ list_del(desc-plist);
+ kfree(desc);
+}
+
+static void release_persistent_event(struct perf_event *event)
+{
+ perf_event_disable(event);
+ perf_event_release_kernel(event);
+}
+
static void del_persistent_event(int cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
@@ -100,12 +111,14 @@ static void del_persistent_event(int cpu, struct
perf_event_attr *attr)
goto out;
event = desc-event;
- list_del(desc-plist);
-
- perf_event_disable(event);
- perf_event_release_kernel(event);
- put_unused_fd(desc-fd);
- kfree(desc);
+ /*
+* We primarily want to remove desc from the list. If there
+* are no open files, the refcount is 0 and we need to release
+* the event too.
+*/
+ detach_persistent_event(desc);
+ if (atomic_long_dec_and_test(event-refcount))
+ release_persistent_event(event);
out:
mutex_unlock(per_cpu(pers_events_lock, cpu));
}
@@ -182,18 +195,31 @@ fail:
int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
{
struct pers_event_desc *desc;
+ struct perf_event *event;
int event_fd = -ENODEV;
mutex_lock(per_cpu(pers_events_lock, cpu));
desc = get_persistent_event(cpu, attr);
- if (!desc)
+
+ /* Increment refcount to keep event on put_event() */
+ if (!desc || !atomic_long_inc_not_zero(desc-event-refcount))
goto out;
event_fd = anon_inode_getfd([pers_event], perf_fops,
desc-event, O_RDONLY);
- if (event_fd = 0)
- desc-fd = event_fd;
+
+ if (event_fd 0) {
+ event = desc-event;
+ if (WARN_ON(atomic_long_dec_and_test(event-refcount))) {
+ /*
+* May not happen since decrementing refcount is
+* protected by pers_events_lock.
+*/
+ detach_persistent_event(desc);
+ release_persistent_event(event);
+ }
+ }
out:
mutex_unlock(per_cpu(pers_events_lock, cpu));
--
1.8.1.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
