* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Jonathan Corbet @ 2018-03-29 13:03 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, luto, jlayton, carlos, viro,
dhowells, simo, eparis, serge, ebiederm, madzcar
In-Reply-To: <20180329090132.r3qfomigkw3hbwbw@madcap2.tricolour.ca>
On Thu, 29 Mar 2018 05:01:32 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:
> > A little detail, but still...
>
> I am understanding that you would prefer more context (as opposed to
> operational detail) in the description, laying out the use case for this
> patch(set)?
No, sorry, "a little detail" was referring to my comment. The use case,
I believe, has been well described.
Thanks,
jon
^ permalink raw reply
* Re: [RFC][PATCH] ipc: Remove IPCMNI
From: Matthew Wilcox @ 2018-03-29 10:56 UTC (permalink / raw)
To: Manfred Spraul
Cc: Davidlohr Bueso, Waiman Long, Michael Kerrisk, Eric W. Biederman,
Luis R. Rodriguez, Kees Cook, linux-kernel, linux-fsdevel,
Andrew Morton, Al Viro, Stanislav Kinsbursky, Linux Containers,
linux-api
In-Reply-To: <3e201de2-bed2-6f7d-0783-700d095142e0@colorfullife.com>
On Thu, Mar 29, 2018 at 10:47:45AM +0200, Manfred Spraul wrote:
> > > > > > This can be implemented trivially with the current code
> > > > > > using idr_alloc_cyclic.
>
> Is there a performance impact?
> Right now, the idr tree is only large if there are lots of objects.
> What happens if we have only 1 object, with id=INT_MAX-1?
The radix tree uses a branching factor of 64 entries (6 bits) per level.
The maximum ID is 31 bits (positive signed 32-bit integer). So the
worst case for a single object is 6 pointer dereferences to find the
object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12
cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2
(30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.
I have plans to make the representation more efficient and bring
the specific case of one element with a high ID down to one pointer
dereference and 2 cachelines, but I have not yet had time to implement
those plans.
>From a memory consumption point of view, 6 layers of tree will consume
6/7 of a page on a 64-bit x86 kernel. I'm aiming to bring that down to
1/7 of a page. We get 7 radix_tree_nodes per 4kB page.
(The old IDR tree had 256 entries per level which would have taken only
four layers to get us to 31 bits, but the cost was getting only 3 layers
per 8kB order-1 page, so we'd've taken 2 + 2/3 page to accomplish the
same goal).
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Richard Guy Briggs @ 2018-03-29 9:01 UTC (permalink / raw)
To: Jonathan Corbet
Cc: ebiederm-aS9lmoZGLiVWk0Htik3J/w, simo-H+wXaHxf7aLQT0dZR+AlfA,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, carlos-H+wXaHxf7aLQT0dZR+AlfA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
eparis-FjpueFixGhCM4zKIHC2jIg, dhowells-H+wXaHxf7aLQT0dZR+AlfA,
madzcar-Re5JQEeQqe8AvxtiuMwx3w, Linux-Audit Mailing List,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
luto-DgEjT+Ai2ygdnm+yROfE0A, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20180328123912.49b11c98-T1hC0tSOHrs@public.gmane.org>
On 2018-03-28 12:39, Jonathan Corbet wrote:
> On Fri, 16 Mar 2018 05:00:28 -0400
> Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
>
> A little detail, but still...
I am understanding that you would prefer more context (as opposed to
operational detail) in the description, laying out the use case for this
patch(set)?
> > +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
> > +{
> > + struct task_struct *parent;
> > + u64 pcontainerid, ccontainerid;
> > +
> > + /* Don't allow to set our own containerid */
> > + if (current == task)
> > + return -EPERM;
> > + /* Don't allow the containerid to be unset */
> > + if (!cid_valid(containerid))
> > + return -EINVAL;
>
> I went looking for cid_valid(), but it turns out you don't add it until
> patch 5. That, I expect, will not be good for bisectability (or patch
> review).
Nice catch, thanks Jon. That is very likely another victim of a git
rebase to re-order afterthoughts in the right place. I'll need to be
more careful of that class of bug, rethink my workflow, or script builds
to verify each commit is compilable.
> Thanks,
>
> jon
- RGB
--
Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH 07/30] aio: add delayed cancel support
From: Christoph Hellwig @ 2018-03-29 8:53 UTC (permalink / raw)
To: Al Viro
Cc: Christoph Hellwig, Avi Kivity, linux-aio, linux-fsdevel, netdev,
linux-api, linux-kernel
In-Reply-To: <20180328163526.GV30522@ZenIV.linux.org.uk>
On Wed, Mar 28, 2018 at 05:35:26PM +0100, Al Viro wrote:
> > ret = vfs_fsync(req->file, req->datasync);
> > - fput(req->file);
> > - aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
> > + if (aio_complete(iocb, ret, 0, 0))
> > + fput(file);
>
> IDGI.
> 1) can aio_complete() ever return false here?
It won't. But sometimes checking the return value and sometimes not
seems like a bad pattern.
> 2) do we ever have aio_kiocb that would not have an associated
> struct file * that needs to be dropped on successful aio_complete()? AFAICS,
> rw, fsync and poll variants all have one, and I'm not sure what kind of
> async IO *could* be done without an opened file.
All have a file assoiated at least right now. As mentioned last time
finding a struct to pass that file would be rather annoying, so we'd either
have to pass it explicitly, or do something nasty like duplicating the
pointer in the aio_kiocb in addition to struct kiocb. Which might not
be that bad after all, as it would only bloat the aio_kiocb and not
struct kiocb used on stack all over.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFC][PATCH] ipc: Remove IPCMNI
From: Manfred Spraul @ 2018-03-29 8:47 UTC (permalink / raw)
To: Davidlohr Bueso, Waiman Long, Michael Kerrisk
Cc: Eric W. Biederman, Luis R. Rodriguez, Kees Cook, linux-kernel,
linux-fsdevel, Andrew Morton, Al Viro, Matthew Wilcox,
Stanislav Kinsbursky, Linux Containers, linux-api
In-Reply-To: <20180329021409.gcjjrmviw2lckbfk@linux-n805>
Hello together,
On 03/29/2018 04:14 AM, Davidlohr Bueso wrote:
> Cc'ing mtk, Manfred and linux-api.
>
> See below.
>
> On Thu, 15 Mar 2018, Waiman Long wrote:
>
>> On 03/15/2018 03:00 PM, Eric W. Biederman wrote:
>>> Waiman Long <longman@redhat.com> writes:
>>>
>>>> On 03/14/2018 08:49 PM, Eric W. Biederman wrote:
>>>>> The define IPCMNI was originally the size of a statically sized
>>>>> array in
>>>>> the kernel and that has long since been removed. Therefore there
>>>>> is no
>>>>> fundamental reason for IPCMNI.
>>>>>
>>>>> The only remaining use IPCMNI serves is as a convoluted way to format
>>>>> the ipc id to userspace. It does not appear that anything except for
>>>>> the CHECKPOINT_RESTORE code even cares about this variety of
>>>>> assignment
>>>>> and the CHECKPOINT_RESTORE code only cares about this weirdness
>>>>> because
>>>>> it has to restore these peculiar ids.
>>>>>
My assumption is that if an array is recreated, it should get a
different id.
a=semget(1234,,);
semctl(a,,IPC_RMID);
b=semget(1234,,);
now a!=b.
Rational: semop() calls only refer to the array by the id.
If there is a stale process in the system that tries to access the "old"
array and the new array has the same id, then the locking gets corrupted.
>>>>> Therefore make the assignment of ipc ids match the description in
>>>>> Advanced Programming in the Unix Environment and assign the next id
>>>>> until INT_MAX is hit then loop around to the lower ids.
>>>>>
Ok, sounds good.
That way we really cycle through INT_MAX, right now a==b would happen
after 128k RMID calls.
>>>>> This can be implemented trivially with the current code using
>>>>> idr_alloc_cyclic.
>>>>>
Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects.
What happens if we have only 1 object, with id=INT_MAX-1?
semop() that do not sleep are fairly fast.
The same applies for msgsnd/msgrcv, if the message is small enough.
@Davidlohr:
Do you know if there are application that frequently call semop() and it
doesn't have to sleep?
From the scalability that was pushed into the kernel, I assume that
this exists.
I have myself only checked postgresql, and postgresql always sleeps.
(and this was long ago)
>>>>> To make it possible to keep checkpoint/restore working I have renamed
>>>>> the sysctls from xxx_next_id to xxx_nextid. That is enough change
>>>>> that
>>>>> a smart CRIU implementation can see that what is exported has
>>>>> changed,
>>>>> and act accordingly. New kernels will be able to restore the old
>>>>> id's.
>>>>>
>>>>> This code still needs some real world testing to verify my
>>>>> assumptions.
>>>>> And some work with the CRIU implementations to actually add the code
>>>>> that deals with the new for of id assignment.
>>>>>
It means that all existing checkpoint/restore application will not work
with a new kernel.
Everyone must first update the checkpoint/restore application, then
update the kernel.
Is this acceptable?
--
Manfred
^ permalink raw reply
* Re: [RFC][PATCH] ipc: Remove IPCMNI
From: Davidlohr Bueso @ 2018-03-29 2:14 UTC (permalink / raw)
To: Waiman Long, Michael Kerrisk
Cc: Manfred Spraul, Kees Cook, linux-api-u79uwXL29TY76Z2rM5mHXA,
Linux Containers, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Matthew Wilcox, Luis R. Rodriguez, Eric W. Biederman,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Stanislav Kinsbursky,
Andrew Morton, Al Viro
In-Reply-To: <7d3a1f93-f8e5-5325-f9a7-0079f7777b6f-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc'ing mtk, Manfred and linux-api.
See below.
On Thu, 15 Mar 2018, Waiman Long wrote:
>On 03/15/2018 03:00 PM, Eric W. Biederman wrote:
>> Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>>
>>> On 03/14/2018 08:49 PM, Eric W. Biederman wrote:
>>>> The define IPCMNI was originally the size of a statically sized array in
>>>> the kernel and that has long since been removed. Therefore there is no
>>>> fundamental reason for IPCMNI.
>>>>
>>>> The only remaining use IPCMNI serves is as a convoluted way to format
>>>> the ipc id to userspace. It does not appear that anything except for
>>>> the CHECKPOINT_RESTORE code even cares about this variety of assignment
>>>> and the CHECKPOINT_RESTORE code only cares about this weirdness because
>>>> it has to restore these peculiar ids.
>>>>
>>>> Therefore make the assignment of ipc ids match the description in
>>>> Advanced Programming in the Unix Environment and assign the next id
>>>> until INT_MAX is hit then loop around to the lower ids.
>>>>
>>>> This can be implemented trivially with the current code using idr_alloc_cyclic.
>>>>
>>>> To make it possible to keep checkpoint/restore working I have renamed
>>>> the sysctls from xxx_next_id to xxx_nextid. That is enough change that
>>>> a smart CRIU implementation can see that what is exported has changed,
>>>> and act accordingly. New kernels will be able to restore the old id's.
>>>>
>>>> This code still needs some real world testing to verify my assumptions.
>>>> And some work with the CRIU implementations to actually add the code
>>>> that deals with the new for of id assignment.
>>>>
>>>> Updates: 03f595668017 ("ipc: add sysctl to specify desired next object id")
>>>> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>>
>>>> Waiman please take a look at this and run it through some tests etc,
>>>> I am pretty certain something like this patch is all you need to do
>>>> to sort out ipc assignment. Not messing with sysctls needed.
>>>>
>>>> include/linux/ipc.h | 2 --
>>>> include/linux/ipc_namespace.h | 1 -
>>>> ipc/ipc_sysctl.c | 6 ++--
>>>> ipc/namespace.c | 11 ++----
>>>> ipc/util.c | 80 ++++++++++---------------------------------
>>>> ipc/util.h | 11 +-----
>>>> 6 files changed, 25 insertions(+), 86 deletions(-)
>>>>
>>>> diff --git a/include/linux/ipc.h b/include/linux/ipc.h
>>>> index 821b2f260992..6cc2df7f7ac9 100644
>>>> --- a/include/linux/ipc.h
>>>> +++ b/include/linux/ipc.h
>>>> @@ -8,8 +8,6 @@
>>>> #include <uapi/linux/ipc.h>
>>>> #include <linux/refcount.h>
>>>>
>>>> -#define IPCMNI 32768 /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
>>>> -
>>>> /* used by in-kernel data structures */
>>>> struct kern_ipc_perm {
>>>> spinlock_t lock;
>>>> diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
>>>> index b5630c8eb2f3..cab33b6a8236 100644
>>>> --- a/include/linux/ipc_namespace.h
>>>> +++ b/include/linux/ipc_namespace.h
>>>> @@ -15,7 +15,6 @@ struct user_namespace;
>>>>
>>>> struct ipc_ids {
>>>> int in_use;
>>>> - unsigned short seq;
>>>> bool tables_initialized;
>>>> struct rw_semaphore rwsem;
>>>> struct idr ipcs_idr;
>>>> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
>>>> index 8ad93c29f511..a599963d58bf 100644
>>>> --- a/ipc/ipc_sysctl.c
>>>> +++ b/ipc/ipc_sysctl.c
>>>> @@ -176,7 +176,7 @@ static struct ctl_table ipc_kern_table[] = {
>>>> },
>>>> #ifdef CONFIG_CHECKPOINT_RESTORE
>>>> {
>>>> - .procname = "sem_next_id",
>>>> + .procname = "sem_nextid",
>>>> .data = &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
>>>> .maxlen = sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
>>>> .mode = 0644,
>>>> @@ -185,7 +185,7 @@ static struct ctl_table ipc_kern_table[] = {
>>>> .extra2 = &int_max,
>>>> },
>>>> {
>>>> - .procname = "msg_next_id",
>>>> + .procname = "msg_nextid",
>>>> .data = &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
>>>> .maxlen = sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
>>>> .mode = 0644,
>>>> @@ -194,7 +194,7 @@ static struct ctl_table ipc_kern_table[] = {
>>>> .extra2 = &int_max,
>>>> },
>>>> {
>>>> - .procname = "shm_next_id",
>>>> + .procname = "shm_nextid",
>>>> .data = &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
>>>> .maxlen = sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
>>>> .mode = 0644,
>>> So you are changing the names of existing sysctl parameters. Will it be
>>> better to add new sysctl to indicate that the rule has changed
>>> instead?
>> In practice I am replacing one set of sysctls with another, that work
>> very similarly but not quite the same. As we can't keep the existing
>> semantics removing the old sysctl seems correct. Likewise adding
>> a new sysctl with slightly changed semantics seems correct.
>>
>> This needs an accompanying patch to CRIU to see which sysctls are
>> available and to change it's behavior based upon that. The practical
>> question is what makes it easiest not to confuse CRIU.
>>
>> Not having the sysctl should be something that CRIU detects today
>> and the old versions should fail gracefully. But testing is needed.
>> Adding a new sysctl to say the behavior has changed and reusing the
>> old names won't have the same effect of disabling existing versions
>> of CRIU.
>
>That is fine as long as CRIU is the only user.
>
>>
>>> I don't know the history why the id management of SysV IPC was designed
>>> in such a convoluted way, but the patch does make sense to me.
>> I don't have the full history and we might wind up finding more as we
>> run this patch through it's paces.
>>
>> The earliest history I know is what I read in Advanced Programming in
>> the Unix Environment (which predates linux). It described the ipc ids
>> as assigned from a counter that wraps. I thought like my patch
>> implements. On closer reading it has a counter that increases each time
>> the slot is used, and then wraps. Exactly like Linux before my patch.
>> *Grrr*
>>
>> The existing structure of the bifurcated is present in Linux 1.0. At
>> that time SHMMNI was 256. SHMMNI was the size of a static array of shm
>> segments. The high 24 bits held a sequence number that was incremented
>> when a segment was removed at the time. Presumably the upper bits were
>> incremented to avoid swiftly reusing the same shm ids.
>>
>> Hmm. I took a quick look at FreeBSD10 and it has the exact same split
>> in the id. So userspace may actually depend upon that split.
>
>Backward compatibility is the part that I am most worry about this
>patch. That is also the reason I asked why the ID is generated in such a
>way.
I share these fears.
Thanks,
Davidlohr
>
>My original thinking was to have an extended mode where the IPCMNI
>becomes 8M from 32k. That will reduce the sequence number from 16 bits
>to 8 bits. The extended mode is enabled by adding, for example, a boot
>option. So this will be an opt-in feature instead of as a default.
>
>>
>> Which comes down to the fundamental question what depends upon what.
>> How do other operating systems like Solaris handle this?
>
>I don't know how Solaris handle this, but I know they support up to 2^24
>shm segments.
>
>>
>> Does any nix flavor support more that 16bits worth of shm segments?
>>
>> The API has been deprecated for the last 20 years and we are still
>> keeping it alive. Sigh.
>>
>> Still there is fundamentally only one thing the kernel can do if we wish
>> to increase the number of shm segments.
>>
>> Please take my patch and test it out and see if you can find anything
>> that cares about the change. Except for needing id reuse to be
>> infrequent I can not imagine that there is anything that cares.
>>
>> It could very reasonably be argued that my when shmmni is < INT_MAX
>> my patch implements a version of the existing algorithm. As we go
>> through all of the possible ids before we reuse any of them.
>>
>> Eric
>>
>Thanks for the patch, I am still thinking about what is the best way to
>handle this.
>
>Cheers,
>Longman
>
>
^ permalink raw reply
* Re: [REVIEW][PATCH 00/11] ipc: Fixing the pid namespace support
From: Davidlohr Bueso @ 2018-03-29 1:12 UTC (permalink / raw)
To: Eric W. Biederman
Cc: esyr-H+wXaHxf7aLQT0dZR+AlfA, jannh-hpIqsD4AKlfQT0dZR+AlfA,
khlebnikov-XoJtRXgx1JseBXzfvpsJ4g,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
prakash.sangappa-QHcLZuEGTsvQT0dZR+AlfA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
luto-DgEjT+Ai2ygdnm+yROfE0A, oleg-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, Nagarathnam Muthusamy,
Pavel Emelyanov
In-Reply-To: <87vadmobdw.fsf_-_-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
On Fri, 23 Mar 2018, Eric W. Biederman wrote:
>Still I would like to see this fixed and I plan on merging this code.
Yes, it needs fixed, but 1) there are pending issues (such as the extra atomics)
and 2) its late in the -rc cycle. Plus this issue has existed for 11 years without
the world ending, so I'm sure we can hold on until at least one more release.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [REVIEW][PATCH 11/11] ipc/sem: Fix semctl(..., GETPID, ...) between pid namespaces
From: Davidlohr Bueso @ 2018-03-29 0:52 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, linux-kernel, linux-api, khlebnikov,
prakash.sangappa, luto, akpm, oleg, serge.hallyn, esyr, jannh,
linux-security-module, Pavel Emelyanov, Nagarathnam Muthusamy
In-Reply-To: <20180323191614.32489-11-ebiederm@xmission.com>
On Fri, 23 Mar 2018, Eric W. Biederman wrote:
>Today the last process to update a semaphore is remembered and
>reported in the pid namespace of that process. If there are processes
>in any other pid namespace querying that process id with GETPID the
>result will be unusable nonsense as it does not make any
>sense in your own pid namespace.
Yeah that sounds pretty wrong.
>
>Due to ipc_update_pid I don't think you will be able to get System V
>ipc semaphores into a troublesome cache line ping-pong. Using struct
>pids from separate process are not a problem because they do not share
>a cache line. Using struct pid from different threads of the same
>process are unlikely to be a problem as the reference count update
>can be avoided.
>
>Further linux futexes are a much better tool for the job of mutual
>exclusion between processes than System V semaphores. So I expect
>programs that are performance limited by their interprocess mutual
>exclusion primitive will be using futexes.
You would be wrong. There are plenty of real workloads out there
that do not use futexes and are care about performance; in the end
futexes are only good for the uncontended cases, it can also
destroy numa boxes if you consider the global hash table. Experience
as shown me that sysvipc sems are quite still used.
>
>So while it is possible that enhancing the storage of the last
>rocess of a System V semaphore from an integer to a struct pid
>will cause a performance regression because of the effect
>of frequently updating the pid reference count. I don't expect
>that to happen in practice.
How's that? Now thanks to ipc_update_pid() for each semop the user
passes, perform_atomic_semop() will do two atomic updates for the
cases where there are multiple processes updating the sem. This is
not uncommon.
Could you please provide some numbers.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [PATCH v8 bpf-next 0/9] bpf, tracing: introduce bpf raw tracepoints
From: Daniel Borkmann @ 2018-03-28 23:59 UTC (permalink / raw)
To: Alexei Starovoitov, davem
Cc: torvalds, peterz, rostedt, mathieu.desnoyers, netdev, kernel-team,
linux-api
In-Reply-To: <20180328190540.370956-1-ast@kernel.org>
On 03/28/2018 09:05 PM, Alexei Starovoitov wrote:
> v7->v8:
> - moved 'u32 num_args' from 'struct tracepoint' into 'struct bpf_raw_event_map'
> that increases memory overhead, but can be optimized/compressed later.
> Now it's zero changes in tracepoint.[ch]
[...]
> The first approach simply added these pointers to sched tracepoints:
> https://lkml.org/lkml/2017/12/14/753
> which Peter nacked.
> Few options were discussed and eventually the discussion converged on
> doing bpf specific tracepoint_probe_register() probe functions.
> Details here:
> https://lkml.org/lkml/2017/12/20/929
>
> Patch 1 is kernel wide cleanup of pass-struct-by-value into
> pass-struct-by-reference into tracepoints.
>
> Patches 2 and 3 are minor cleanups to address allyesconfig build
>
> Patch 4 refactor trace_iwlwifi_dev_ucode_error from 17 to 4 args
>
> Patch 5 introduces COUNT_ARGS macro
>
> Patch 6 introduces BPF_RAW_TRACEPOINT api.
> the auto-cleanup and multiple concurrent users are must have
> features of tracing api. For bpf raw tracepoints it looks like:
> // load bpf prog with BPF_PROG_TYPE_RAW_TRACEPOINT type
> prog_fd = bpf_prog_load(...);
>
> // receive anon_inode fd for given bpf_raw_tracepoint
> // and attach bpf program to it
> raw_tp_fd = bpf_raw_tracepoint_open("xdp_exception", prog_fd);
>
> Ctrl-C of tracing daemon or cmdline tool will automatically
> detach bpf program, unload it and unregister tracepoint probe.
> More details in patch 6.
>
> Patch 7 - trivial support in libbpf
> Patches 8, 9 - user space tests
>
> samples/bpf/test_overhead performance on 1 cpu:
>
> tracepoint base kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
> task_rename 1.1M 769K 947K 1.0M
> urandom_read 789K 697K 750K 755K
Applied to bpf-next, thanks everyone!
^ permalink raw reply
* Re: [REVIEW][PATCH 13/11] ipc/smack: Tidy up from the change in type of the ipc security hooks
From: Davidlohr Bueso @ 2018-03-28 23:57 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, esyr, jannh, khlebnikov, linux-api,
serge.hallyn, linux-kernel, prakash.sangappa,
linux-security-module, luto, oleg, akpm, Nagarathnam Muthusamy,
Pavel Emelyanov
In-Reply-To: <87y3iikp1y.fsf_-_@xmission.com>
On Sat, 24 Mar 2018, Eric W. Biederman wrote:
> /**
>- * smack_of_shm - the smack pointer for the shm
>- * @shp: the object
>+ * smack_of_ipc - the smack pointer for the ipc
>+ * @isp: the object
Nit, but while at it the @isp description does need some love:
"@isp: the pointer for the ipc perm structure"
^ permalink raw reply
* Re: [REVIEW][PATCH 12/11] ipc: Directly call the security hook in ipc_ops.associate
From: Davidlohr Bueso @ 2018-03-28 23:40 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Containers, esyr, jannh, khlebnikov, linux-api,
serge.hallyn, linux-kernel, prakash.sangappa,
linux-security-module, luto, oleg, akpm, Nagarathnam Muthusamy,
Pavel Emelyanov
In-Reply-To: <877eq2m3or.fsf_-_@xmission.com>
On Sat, 24 Mar 2018, Eric W. Biederman wrote:
>
>After the last round of cleanups the shm, sem, and msg associate
>operations just became trivial wrappers around the appropriate security
>method. Simplify things further by just calling the security method
>directly.
>
>Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>---
> ipc/msg.c | 10 +---------
> ipc/sem.c | 10 +---------
> ipc/shm.c | 10 +---------
> 3 files changed, 3 insertions(+), 27 deletions(-)
This is nice to see.
^ permalink raw reply
* Re: [REVIEW][PATCH 13/11] ipc/smack: Tidy up from the change in type of the ipc security hooks
From: Davidlohr Bueso @ 2018-03-28 23:38 UTC (permalink / raw)
To: Casey Schaufler
Cc: Eric W. Biederman, Linux Containers, esyr, jannh, khlebnikov,
linux-api, serge.hallyn, linux-kernel, prakash.sangappa,
linux-security-module, luto, oleg, akpm, Nagarathnam Muthusamy,
Pavel Emelyanov, Paul Moore
In-Reply-To: <80cd2fea-c9a8-4f26-acbb-e0ecb34e4e40@schaufler-ca.com>
On Sat, 24 Mar 2018, Casey Schaufler wrote:
>On 3/23/2018 10:42 PM, Eric W. Biederman wrote:
>> Rename the variables shp, sma, msq to isp. As that is how the code already
>> refers to those variables.
>
>Thanks. It's important to keep the code readable.
Ah great, ignore my last email then.
^ permalink raw reply
* Re: [REVIEW][PATCH 01/11] sem/security: Pass kern_ipc_perm not sem_array into the sem security hooks
From: Davidlohr Bueso @ 2018-03-28 23:20 UTC (permalink / raw)
To: Casey Schaufler
Cc: Eric W. Biederman, Linux Containers, linux-kernel, linux-api,
khlebnikov, prakash.sangappa, luto, akpm, oleg, serge.hallyn,
esyr, jannh, linux-security-module, Pavel Emelyanov,
Nagarathnam Muthusamy
In-Reply-To: <bdf6ed62-b75c-1920-d5ce-ea08428d03d0@schaufler-ca.com>
On Fri, 23 Mar 2018, Casey Schaufler wrote:
>A kern_ipc_perm pointer is conventionally named isp in this code.
So the ideal name would be ipcp, used in core ipc, but I have no strong
preference over isp, ipp or whatever other name is used in LSMs. The
important thing is that kern_ipc_perm should not be called sma or any
ipc specific name.
>How about instead:
Agreed.
>
>-static int smack_sem_alloc_security(struct sem_array *sma)
>+static int smack_sem_alloc_security(struct kern_ipc_perm *isp)
> {
>- struct kern_ipc_perm *isp = &sma->sem_perm;
>
Thanks,
Davidlohr
^ permalink raw reply
* Re: [REVIEW][PATCH 09/11] ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
From: Nagarathnam Muthusamy @ 2018-03-28 23:18 UTC (permalink / raw)
To: Eric W. Biederman
Cc: esyr-H+wXaHxf7aLQT0dZR+AlfA, jannh-hpIqsD4AKlfQT0dZR+AlfA,
khlebnikov-XoJtRXgx1JseBXzfvpsJ4g,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
prakash.sangappa-QHcLZuEGTsvQT0dZR+AlfA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
luto-DgEjT+Ai2ygdnm+yROfE0A, oleg-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, Pavel Emelyanov
In-Reply-To: <87woxvajk9.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
On 03/28/2018 04:04 PM, ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org wrote:
> NAGARATHNAM MUTHUSAMY <nagarathnam.muthusamy-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> writes:
>
>> On 3/23/2018 2:33 PM, ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org wrote:
>>> NAGARATHNAM MUTHUSAMY <nagarathnam.muthusamy-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> writes:
>>>
>>>> Thanks!
>>>>
>>>> Reviewed-by: Nagarathnam Muthusamy <nagarathnam.muthusamy-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>>> Does this look like it will address the issue you have been fighting
>>> with pids?
>> We do use IPC shared memory but it is a single large one, shared by multiple
>> levels. We are currently looking into using a similar solution based on file
>> locks.
>> When a new level is created, a file representing that level could be created in
>> a common path which could be locked by the init process of that level.
>> Parent levels could query the locking pid of that file to get the pid
>> translation
>> of the init process of the required level. Then it could open a file descriptor
>> and use the translate_pid API for further translations.
> Do you want to resend the translate_pid API with file descriptors as it
> was in the lwn article? That I will apply.
Sure Eric! We are currently implementing and testing the file locks + FD
based
approach, just to make sure it covers all the requirements. Will resend the
patch with FD based translate_pid API in few days.
Thanks,
Nagarathnam.
>
> Eric
^ permalink raw reply
* Re: [REVIEW][PATCH 09/11] ipc/shm: Fix shmctl(..., IPC_STAT, ...) between pid namespaces.
From: Eric W. Biederman @ 2018-03-28 23:04 UTC (permalink / raw)
To: NAGARATHNAM MUTHUSAMY
Cc: Linux Containers, linux-kernel, linux-api, khlebnikov,
prakash.sangappa, luto, akpm, oleg, serge.hallyn, esyr, jannh,
linux-security-module, Pavel Emelyanov
In-Reply-To: <1091a91e-f8ee-b091-6d95-78b33520fb2d@oracle.com>
NAGARATHNAM MUTHUSAMY <nagarathnam.muthusamy@oracle.com> writes:
> On 3/23/2018 2:33 PM, ebiederm@xmission.com wrote:
>> NAGARATHNAM MUTHUSAMY <nagarathnam.muthusamy@oracle.com> writes:
>>
>>> Thanks!
>>>
>>> Reviewed-by: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
>> Does this look like it will address the issue you have been fighting
>> with pids?
>
> We do use IPC shared memory but it is a single large one, shared by multiple
> levels. We are currently looking into using a similar solution based on file
> locks.
> When a new level is created, a file representing that level could be created in
> a common path which could be locked by the init process of that level.
> Parent levels could query the locking pid of that file to get the pid
> translation
> of the init process of the required level. Then it could open a file descriptor
> and use the translate_pid API for further translations.
Do you want to resend the translate_pid API with file descriptors as it
was in the lwn article? That I will apply.
Eric
^ permalink raw reply
* Re: [PATCH 07/30] aio: add delayed cancel support
From: Al Viro @ 2018-03-28 21:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180328163526.GV30522@ZenIV.linux.org.uk>
On Wed, Mar 28, 2018 at 05:35:26PM +0100, Al Viro wrote:
> On Wed, Mar 28, 2018 at 09:29:03AM +0200, Christoph Hellwig wrote:
> > static void aio_fsync_work(struct work_struct *work)
> > {
> > struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
> > + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, fsync);
> > + struct file *file = req->file;
> > int ret;
> >
> > ret = vfs_fsync(req->file, req->datasync);
> > - fput(req->file);
> > - aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
> > + if (aio_complete(iocb, ret, 0, 0))
> > + fput(file);
>
> IDGI.
> 1) can aio_complete() ever return false here?
> 2) do we ever have aio_kiocb that would not have an associated
> struct file * that needs to be dropped on successful aio_complete()? AFAICS,
> rw, fsync and poll variants all have one, and I'm not sure what kind of
> async IO *could* be done without an opened file.
OK, hell with that. I've tried to play with turning kiocb into a struct with
anon union in it, with poll and fsync parts folded into that sucker and ki_filp
lifted into common part. Possible, but it's hairy as hell and can be done
afterwards.
However, doing that digging has turned up something really nasty. Look:
in io_cancel(2) you have
spin_lock_irq(&ctx->ctx_lock);
kiocb = lookup_kiocb(ctx, iocb, key);
if (kiocb) {
if (kiocb->flags & AIO_IOCB_DELAYED_CANCEL) {
kiocb->flags |= AIO_IOCB_CANCELLED;
} else {
ret = kiocb_cancel(kiocb);
kiocb = NULL;
}
}
spin_unlock_irq(&ctx->ctx_lock);
Now, suppose two threads call io_cancel() on the same aio_poll in progress.
Both hit that code and *both* find the same kiocb. Sure, the first one
will shortly do
if (kiocb)
ret = kiocb_cancel(kiocb);
which will remove it from the list. Too late, though - you've already dropped
->ctx_lock, letting the second one find it. Result: two aio_poll_cancel() in
parallel, with resulting double-free and double-fput().
You really need to remove it from the ->active_reqs before dropping the lock.
free_ioctx_users() does it correctly, io_cancel(2) fucks it up.
I'd add something like
struct aio_kiocb *kiocb_cancel_locked(struct aio_kiocb *kiocb)
{
if (!kiocb)
return ERR_PTR(-EINVAL);
if (kiocb->flags & AIO_IOCB_DELAYED_CANCEL) {
list_del(&kiocb->ki_list);
kiocb->flags |= AIO_IOCB_CANCELLED;
return kiocb;
} else {
return ERR_PTR(kiocb_cancel(kiocb));
}
}
with
spin_lock_irq(&ctx->ctx_lock);
while (!list_empty(&ctx->active_reqs)) {
req = list_first_entry(&ctx->active_reqs,
struct aio_kiocb, ki_list);
req = kiocb_cancel_locked(req);
if (!IS_ERR_OR_NULL(req))
list_add_tail(&req->ki_list, &list);
}
spin_unlock_irq(&ctx->ctx_lock);
in free_ioctx_users() and
spin_lock_irq(&ctx->ctx_lock);
kiocb = kiocb_cancel_locked(lookup_kiocb(ctx, iocb, key));
spin_unlock_irq(&ctx->ctx_lock);
ret = IS_ERR_OR_NULL(kiocb) ? PTR_ERR(kiocb) : kiocb_cancel(kiocb);
in io_cancel(2)...
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Thomas Gleixner @ 2018-03-28 21:25 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <wil>
In-Reply-To: <181076499.279.1522268382303.JavaMail.zimbra@efficios.com>
On Wed, 28 Mar 2018, Mathieu Desnoyers wrote:
> ----- On Mar 28, 2018, at 1:49 PM, Peter Zijlstra peterz@infradead.org wrote:
> > I don't think disallowing system calls is arbitrary. And I think that is
> > something we really want to enforce, because it's batshit insane to
> > allow.
> >
> > And if we allow now, people _will_ use it and we can't ever take it
> > away again.
>
> Here are some examples of how I would like to use system calls within
> rseq critical sections, for testing purposes:
>
> - Issue poll(NULL, 0, ms_timeout) from a rseq critical section, to introduce
> a delay in the critical section and test the effect,
It's simple enough to use a delay loop for that. It's testing after all.
> - Issue sched_yield() from a rseq critical section, to introduce preemption at
> that point,
Make it loop on a varible and use secondary threads to force preemption.
> - Issue kill() on self, thus testing interruption by signals over rseq c.s.,
Second thread can do that
> - Invoke sched_setaffinity to tweak the cpu affinity mask to force thread
> migration within a rseq c.s.
Second thread can do that
> I currently have only implemented the poll(), sched_yield() and kill()
> test-cases outside of the rseq critical sections, instead relying on
> assembly loops to introduce delays in rseq c.s.. However, if we disallow
> system calls in rseq critical sections, I'll never be able to use those
> systems calls to extend the test matrix.
All of these tests can be implemented without system calls and there is no
justification to allow system calls just because it makes writing test
cases simpler. Nice try.
> I see other use-cases where having a system call in a rseq critical section
> could make sense: if vDSO data shared between kernel and user-space rely
> on rseq for synchronization, but a fallback sometimes needs to issue a system
> call for part of the operation.
What in the VDSO relies on rseqs? Nothing AFAICT. If the VDSO ever goes to
use that then it's going to be a kernel/vdso specific variant and we'll
figure out how that needs to be handled if at all.
But we are not misdesigning now to accomodate artificial scenarios dreamed
up for argumentation sake,
> Therefore I'd really want to keep allowing system calls within rseq critical
> sections, even though we don't expect this to be the typical use-case.
Syscalls inside rseq sections make no sense whatsoever, unless you can
rollback the message you just sent through the intertubes when the rseq
loop failed the taste test. If that works we might reconsider.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-28 20:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, Russell King,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180328174935.GK4082@hirez.programming.kicks-ass.net>
----- On Mar 28, 2018, at 1:49 PM, Peter Zijlstra peterz@infradead.org wrote:
> On Wed, Mar 28, 2018 at 11:37:06AM -0400, Mathieu Desnoyers wrote:
>> ----- On Mar 28, 2018, at 11:28 AM, Peter Zijlstra peterz@infradead.org wrote:
>>
>> > On Wed, Mar 28, 2018 at 11:14:05AM -0400, Mathieu Desnoyers wrote:
>> >
>> >> > If at all possible I would make it SIGSEGV when issueing SYSCALL()s from
>> >> > within an RSEQ.
>> >>
>> >> What's the goal there ? rseq critical sections can technically do system calls
>> >> if they wish. Why prevent this ?
>> >
>> > This all started as a way to do 'small' _fast_ per-cpu ops, System calls
>> > do NOT fit in that pattern. If you're willing to do a system calls the
>> > cost of atomics is not a problem.
>>
>> I'm not arguing that a typical rseq would do a system call. I'm merely
>> pointing out that if we start putting arbitrary limitations like "SIGSEGV
>> when a fork or system call is encountered on top of rseq", this will cause
>> pain in user-space.
>
> I don't think disallowing system calls is arbitrary. And I think that is
> something we really want to enforce, because it's batshit insane to
> allow.
>
> And if we allow now, people _will_ use it and we can't ever take it
> away again.
Here are some examples of how I would like to use system calls within
rseq critical sections, for testing purposes:
- Issue poll(NULL, 0, ms_timeout) from a rseq critical section, to introduce
a delay in the critical section and test the effect,
- Issue sched_yield() from a rseq critical section, to introduce preemption at
that point,
- Issue kill() on self, thus testing interruption by signals over rseq c.s.,
- Invoke sched_setaffinity to tweak the cpu affinity mask to force thread
migration within a rseq c.s.
I currently have only implemented the poll(), sched_yield() and kill()
test-cases outside of the rseq critical sections, instead relying on
assembly loops to introduce delays in rseq c.s.. However, if we disallow
system calls in rseq critical sections, I'll never be able to use those
systems calls to extend the test matrix.
I see other use-cases where having a system call in a rseq critical section
could make sense: if vDSO data shared between kernel and user-space rely
on rseq for synchronization, but a fallback sometimes needs to issue a system
call for part of the operation.
Therefore I'd really want to keep allowing system calls within rseq critical
sections, even though we don't expect this to be the typical use-case.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Mathieu Desnoyers @ 2018-03-28 19:48 UTC (permalink / raw)
To: rostedt
Cc: Alexei Starovoitov, Alexei Starovoitov, David S. Miller,
Daniel Borkmann, Linus Torvalds, Peter Zijlstra, netdev,
kernel-team, linux-api
In-Reply-To: <20180328154027.569206ba@gandalf.local.home>
----- On Mar 28, 2018, at 3:40 PM, rostedt rostedt@goodmis.org wrote:
> On Wed, 28 Mar 2018 12:38:48 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> On 3/28/18 12:34 PM, Steven Rostedt wrote:
>> > On Wed, 28 Mar 2018 12:05:37 -0700
>> > Alexei Starovoitov <ast@kernel.org> wrote:
>> >
>> >> +++ b/include/linux/tracepoint-defs.h
>> >> @@ -35,4 +35,10 @@ struct tracepoint {
>> >> struct tracepoint_func __rcu *funcs;
>> >> };
>> >>
>> >> +struct bpf_raw_event_map {
>> >> + struct tracepoint *tp;
>> >> + void *bpf_func;
>> >> + u32 num_args;
>> >> +} __aligned(32);
>> >> +
>> >
>> > If you prefer v7, I'm fine with that. For cache issues, I can pull out
>> > the funcs from the tracepoint structure like I posted.
>>
>> I very much prefer to land this v8 as-is and optimize later.
>>
>> I still have bpfilter/microkernel patches to finish which were
>> practically ready two weeks ago and got delayed but this set.
>
> Then by all means, you have my Ack. We can optimize later.
>
> For the whole series... (v7 or v8)
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
I'm fine with either v7 or v8 as well.
Thanks,
Mathieu
>
> Thanks!
>
> -- Steve
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-28 19:47 UTC (permalink / raw)
To: rostedt
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328153819.654c1600@gandalf.local.home>
----- On Mar 28, 2018, at 3:38 PM, rostedt rostedt@goodmis.org wrote:
> On Wed, 28 Mar 2018 15:32:20 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> -#define __DO_TRACE(tp, proto, args, cond, rcucheck) \
>> +#define __DO_TRACE(name, proto, args, cond, rcucheck) \
>> do { \
>> struct tracepoint_func *it_func_ptr; \
>> void *it_func; \
>> @@ -140,7 +140,7 @@ extern void syscall_unregfunc(void);
>> if (rcucheck) \
>> rcu_irq_enter_irqson(); \
>> rcu_read_lock_sched_notrace(); \
>> - it_func_ptr = rcu_dereference_sched((tp)->funcs); \
>> + it_func_ptr = rcu_dereference_sched(__trace_##name##_funcs); \
>
> What we lose in data size, we may make up for in text (which is even
> more important). This will remove a dereference in the hot path.
>
> I'll make a few builds and run size on the vmlinux images to see how
> this pans out.
I like the approach. If it passes testing, I think it's a valuable improvement
to lessen cache footprint of tracepoint when tracing is active.
Thanks!
Mathieu
>
> -- Steve
>
>
>> if (it_func_ptr) { \
> > do { \
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-28 19:40 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, davem, daniel, torvalds, peterz,
mathieu.desnoyers, netdev, kernel-team, linux-api
In-Reply-To: <2a0f6ac2-b13e-b8ad-31b4-a42909a34efa@fb.com>
On Wed, 28 Mar 2018 12:38:48 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> On 3/28/18 12:34 PM, Steven Rostedt wrote:
> > On Wed, 28 Mar 2018 12:05:37 -0700
> > Alexei Starovoitov <ast@kernel.org> wrote:
> >
> >> +++ b/include/linux/tracepoint-defs.h
> >> @@ -35,4 +35,10 @@ struct tracepoint {
> >> struct tracepoint_func __rcu *funcs;
> >> };
> >>
> >> +struct bpf_raw_event_map {
> >> + struct tracepoint *tp;
> >> + void *bpf_func;
> >> + u32 num_args;
> >> +} __aligned(32);
> >> +
> >
> > If you prefer v7, I'm fine with that. For cache issues, I can pull out
> > the funcs from the tracepoint structure like I posted.
>
> I very much prefer to land this v8 as-is and optimize later.
>
> I still have bpfilter/microkernel patches to finish which were
> practically ready two weeks ago and got delayed but this set.
Then by all means, you have my Ack. We can optimize later.
For the whole series... (v7 or v8)
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Thanks!
-- Steve
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Alexei Starovoitov @ 2018-03-28 19:38 UTC (permalink / raw)
To: Steven Rostedt, Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev,
kernel-team, linux-api
In-Reply-To: <20180328153435.5e733957@gandalf.local.home>
On 3/28/18 12:34 PM, Steven Rostedt wrote:
> On Wed, 28 Mar 2018 12:05:37 -0700
> Alexei Starovoitov <ast@kernel.org> wrote:
>
>> +++ b/include/linux/tracepoint-defs.h
>> @@ -35,4 +35,10 @@ struct tracepoint {
>> struct tracepoint_func __rcu *funcs;
>> };
>>
>> +struct bpf_raw_event_map {
>> + struct tracepoint *tp;
>> + void *bpf_func;
>> + u32 num_args;
>> +} __aligned(32);
>> +
>
> If you prefer v7, I'm fine with that. For cache issues, I can pull out
> the funcs from the tracepoint structure like I posted.
I very much prefer to land this v8 as-is and optimize later.
I still have bpfilter/microkernel patches to finish which were
practically ready two weeks ago and got delayed but this set.
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 19:38 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328153220.06a132f4@gandalf.local.home>
On Wed, 28 Mar 2018 15:32:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> -#define __DO_TRACE(tp, proto, args, cond, rcucheck) \
> +#define __DO_TRACE(name, proto, args, cond, rcucheck) \
> do { \
> struct tracepoint_func *it_func_ptr; \
> void *it_func; \
> @@ -140,7 +140,7 @@ extern void syscall_unregfunc(void);
> if (rcucheck) \
> rcu_irq_enter_irqson(); \
> rcu_read_lock_sched_notrace(); \
> - it_func_ptr = rcu_dereference_sched((tp)->funcs); \
> + it_func_ptr = rcu_dereference_sched(__trace_##name##_funcs); \
What we lose in data size, we may make up for in text (which is even
more important). This will remove a dereference in the hot path.
I'll make a few builds and run size on the vmlinux images to see how
this pans out.
-- Steve
> if (it_func_ptr) { \
> do { \
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-28 19:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev,
kernel-team, linux-api
In-Reply-To: <20180328190540.370956-7-ast@kernel.org>
On Wed, 28 Mar 2018 12:05:37 -0700
Alexei Starovoitov <ast@kernel.org> wrote:
> +++ b/include/linux/tracepoint-defs.h
> @@ -35,4 +35,10 @@ struct tracepoint {
> struct tracepoint_func __rcu *funcs;
> };
>
> +struct bpf_raw_event_map {
> + struct tracepoint *tp;
> + void *bpf_func;
> + u32 num_args;
> +} __aligned(32);
> +
If you prefer v7, I'm fine with that. For cache issues, I can pull out
the funcs from the tracepoint structure like I posted.
Mathieu, your thoughts?
-- Steve
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 19:32 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <842190155.225.1522264944386.JavaMail.zimbra@efficios.com>
On Wed, 28 Mar 2018 15:22:24 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > > cache hot/cold argument clearly doesn't apply.
>
> In the current situation I'm fine with adding this extra field
> to struct tracepoint. However, we should keep in mind to move
> all non-required cache-cold fields to a separate section at
> some point. Clearly just this single field won't make a difference
> due to other fields and padding.
funcs is the only part of the tracepoint structure that needs hot
cache. Thus, instead of trying to keep the tracepoint structure small
for cache reasons, pull funcs out of the tracepoint structure.
What about this patch?
Of course, this just adds another 8 bytes per tracepoint :-/ But it
keeps the functions away from the tracepoint structures. We could even
add a section for them to be together, but I'm not sure that will help
much more that just letting the linker place them, as these function
pointers of the same system will probably be grouped together.
-- Steve
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 35db8dd48c4c..8d100163e9af 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -32,7 +32,7 @@ struct tracepoint {
struct static_key key;
int (*regfunc)(void);
void (*unregfunc)(void);
- struct tracepoint_func __rcu *funcs;
+ struct tracepoint_func **funcs;
u32 num_args;
};
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index c92f4adbc0d7..b55282202f71 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -129,7 +129,7 @@ extern void syscall_unregfunc(void);
* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just
* "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto".
*/
-#define __DO_TRACE(tp, proto, args, cond, rcucheck) \
+#define __DO_TRACE(name, proto, args, cond, rcucheck) \
do { \
struct tracepoint_func *it_func_ptr; \
void *it_func; \
@@ -140,7 +140,7 @@ extern void syscall_unregfunc(void);
if (rcucheck) \
rcu_irq_enter_irqson(); \
rcu_read_lock_sched_notrace(); \
- it_func_ptr = rcu_dereference_sched((tp)->funcs); \
+ it_func_ptr = rcu_dereference_sched(__trace_##name##_funcs); \
if (it_func_ptr) { \
do { \
it_func = (it_func_ptr)->func; \
@@ -158,7 +158,7 @@ extern void syscall_unregfunc(void);
static inline void trace_##name##_rcuidle(proto) \
{ \
if (static_key_false(&__tracepoint_##name.key)) \
- __DO_TRACE(&__tracepoint_##name, \
+ __DO_TRACE(name, \
TP_PROTO(data_proto), \
TP_ARGS(data_args), \
TP_CONDITION(cond), 1); \
@@ -181,10 +181,11 @@ extern void syscall_unregfunc(void);
*/
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
extern struct tracepoint __tracepoint_##name; \
+ extern struct tracepoint_func __rcu *__trace_##name##_funcs; \
static inline void trace_##name(proto) \
{ \
if (static_key_false(&__tracepoint_##name.key)) \
- __DO_TRACE(&__tracepoint_##name, \
+ __DO_TRACE(name, \
TP_PROTO(data_proto), \
TP_ARGS(data_args), \
TP_CONDITION(cond), 0); \
@@ -233,9 +234,11 @@ extern void syscall_unregfunc(void);
#define DEFINE_TRACE_FN(name, reg, unreg, num_args) \
static const char __tpstrtab_##name[] \
__attribute__((section("__tracepoints_strings"))) = #name; \
+ struct tracepoint_func __rcu *__trace_##name##_funcs; \
struct tracepoint __tracepoint_##name \
__attribute__((section("__tracepoints"))) = \
- { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
+ { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, \
+ &__trace_##name##_funcs, num_args }; \
static struct tracepoint * const __tracepoint_ptr_##name __used \
__attribute__((section("__tracepoints_ptrs"))) = \
&__tracepoint_##name;
@@ -244,9 +247,12 @@ extern void syscall_unregfunc(void);
DEFINE_TRACE_FN(name, NULL, NULL, num_args);
#define EXPORT_TRACEPOINT_SYMBOL_GPL(name) \
- EXPORT_SYMBOL_GPL(__tracepoint_##name)
+ EXPORT_SYMBOL_GPL(__tracepoint_##name); \
+ EXPORT_SYMBOL_GPL(__trace_##name##_funcs)
+
#define EXPORT_TRACEPOINT_SYMBOL(name) \
- EXPORT_SYMBOL(__tracepoint_##name)
+ EXPORT_SYMBOL(__tracepoint_##name); \
+ EXPORT_SYMBOL(__trace_##name##_funcs)
#else /* !TRACEPOINTS_ENABLED */
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index a02bc09d765a..47b884809f22 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -809,6 +809,7 @@ static void free_synth_tracepoint(struct tracepoint *tp)
if (!tp)
return;
+ kfree(tp->funcs);
kfree(tp->name);
kfree(tp);
}
@@ -827,6 +828,13 @@ static struct tracepoint *alloc_synth_tracepoint(char *name)
return ERR_PTR(-ENOMEM);
}
+ tp->funcs = kzalloc(sizeof(*tp->funcs), GFP_KERNEL);
+ if (!tp->funcs) {
+ kfree(tp->name);
+ kfree(tp);
+ return ERR_PTR(-ENOMEM);
+ }
+
return tp;
}
@@ -846,7 +854,7 @@ static inline void trace_synth(struct synth_event *event, u64 *var_ref_vals,
if (!(cpu_online(raw_smp_processor_id())))
return;
- probe_func_ptr = rcu_dereference_sched((tp)->funcs);
+ probe_func_ptr = rcu_dereference_sched(*(tp)->funcs);
if (probe_func_ptr) {
do {
probe_func = probe_func_ptr->func;
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 671b13457387..638a35f77841 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -203,7 +203,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
return ret;
}
- tp_funcs = rcu_dereference_protected(tp->funcs,
+ tp_funcs = rcu_dereference_protected(*tp->funcs,
lockdep_is_held(&tracepoints_mutex));
old = func_add(&tp_funcs, func, prio);
if (IS_ERR(old)) {
@@ -217,7 +217,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
* a pointer to it. This array is referenced by __DO_TRACE from
* include/linux/tracepoint.h using rcu_dereference_sched().
*/
- rcu_assign_pointer(tp->funcs, tp_funcs);
+ rcu_assign_pointer(*tp->funcs, tp_funcs);
if (!static_key_enabled(&tp->key))
static_key_slow_inc(&tp->key);
release_probes(old);
@@ -235,7 +235,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
{
struct tracepoint_func *old, *tp_funcs;
- tp_funcs = rcu_dereference_protected(tp->funcs,
+ tp_funcs = rcu_dereference_protected(*tp->funcs,
lockdep_is_held(&tracepoints_mutex));
old = func_remove(&tp_funcs, func);
if (IS_ERR(old)) {
@@ -251,7 +251,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
if (static_key_enabled(&tp->key))
static_key_slow_dec(&tp->key);
}
- rcu_assign_pointer(tp->funcs, tp_funcs);
+ rcu_assign_pointer(*tp->funcs, tp_funcs);
release_probes(old);
return 0;
}
@@ -398,7 +398,7 @@ static void tp_module_going_check_quiescent(struct tracepoint * const *begin,
if (!begin)
return;
for (iter = begin; iter < end; iter++)
- WARN_ON_ONCE((*iter)->funcs);
+ WARN_ON_ONCE(*(*iter)->funcs);
}
static int tracepoint_module_coming(struct module *mod)
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox