From: David Howells <dhowells@redhat.com> To: Greg KH <gregkh@linuxfoundation.org> Cc: dhowells@redhat.com, viro@zeniv.linux.org.uk, raven@themaw.net, linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org, linux-block@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer Date: Wed, 29 May 2019 17:06:40 +0100 [thread overview] Message-ID: <31936.1559146000@warthog.procyon.org.uk> (raw) In-Reply-To: <20190528231218.GA28384@kroah.com> Greg KH <gregkh@linuxfoundation.org> wrote: > > kref_put() could potentially add an unnecessary extra stack frame and would > > seem to be best avoided, though an optimising compiler ought to be able to > > inline if it can. > > If kref_put() is on your fast path, you have worse problems (kfree isn't > fast, right?) > > Anyway, it's an inline function, how can it add an extra stack frame? The call to the function pointer. Hopefully the compiler will optimise that away for an inlineable function. > > Are you now on the convert all refcounts to krefs path? > > "now"? Remember, I wrote kref all those years ago, Yes - and I thought it wasn't a good idea at the time. But this is the first time you've mentioned it to me, let alone pushed to change to it, that I recall. > everyone should use > it. It saves us having to audit the same pattern over and over again. > And, even nicer, it uses a refcount now, and as you are trying to > reference count an object, it is exactly what this was written for. > > So yes, I do think it should be used here, unless it is deemed to not > fit the pattern/usage model. kref_put() enforces a very specific destructor signature. I know of places where that doesn't work because the destructor takes more than one argument (granted that this is not the case here). So why does kref_put() exist at all? Why not kref_dec_and_test()? Why doesn't refcount_t get merged into kref, or vice versa? Having both would seem redundant. Mind you, I've been gradually reverting atomic_t-to-refcount_t conversions because it seems I'm not allowed refcount_inc/dec_return() and I want to get at the point refcount for tracing purposes. > > > > +module_exit(watch_queue_exit); > > > > > > module_misc_device()? > > > > warthog>git grep module_misc_device -- Documentation/ > > warthog1> > > Do I have to document all helper macros? If you add an API, documenting it is your privilege ;-) It's an important test of the API - if you can't describe it, it's probably wrong. Now I will grant that you didn't add that function... > Anyway, it saves you boilerplate code, but if built in, it's at the module > init level, not the fs init level, like you are asking for here. So that > might not work, it's your call. Actually, I probably shouldn't have a module exit function. It can't be a module as it's called by core code. I'll switch to builtin_misc_device(). > And how does the tracing and perf ring buffers do this without needing > volatile? Why not use the same type of interface they provide, as it's > always good to share code that has already had all of the nasty corner > cases worked out. I've no idea how trace does it - or even where - or even if. As far as I can see, grepping for mmap in kernel/trace/*, there's no mmap support. Reading Documentation/trace/ring-buffer-design.txt the trace subsystem has some sort of transient page fifo which is a lot more complicated than what I want and doesn't look like it'll be mmap'able. Looking at the perf ring buffer, there appears to be a missing barrier in perf_aux_output_end(): rb->user_page->aux_head = rb->aux_head; should be: smp_store_release(&rb->user_page->aux_head, rb->aux_head); It should also be using smp_load_acquire(). See Documentation/core-api/circular-buffers.rst And a (partial) patch has been proposed: https://lkml.org/lkml/2018/5/10/249 David
WARNING: multiple messages have this Message-ID (diff)
From: David Howells <dhowells@redhat.com> To: Greg KH <gregkh@linuxfoundation.org> Cc: dhowells@redhat.com, viro@zeniv.linux.org.uk, raven@themaw.net, linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org, linux-block@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/7] General notification queue with user mmap()'able ring buffer Date: Wed, 29 May 2019 16:06:40 +0000 [thread overview] Message-ID: <31936.1559146000@warthog.procyon.org.uk> (raw) In-Reply-To: <20190528231218.GA28384@kroah.com> Greg KH <gregkh@linuxfoundation.org> wrote: > > kref_put() could potentially add an unnecessary extra stack frame and would > > seem to be best avoided, though an optimising compiler ought to be able to > > inline if it can. > > If kref_put() is on your fast path, you have worse problems (kfree isn't > fast, right?) > > Anyway, it's an inline function, how can it add an extra stack frame? The call to the function pointer. Hopefully the compiler will optimise that away for an inlineable function. > > Are you now on the convert all refcounts to krefs path? > > "now"? Remember, I wrote kref all those years ago, Yes - and I thought it wasn't a good idea at the time. But this is the first time you've mentioned it to me, let alone pushed to change to it, that I recall. > everyone should use > it. It saves us having to audit the same pattern over and over again. > And, even nicer, it uses a refcount now, and as you are trying to > reference count an object, it is exactly what this was written for. > > So yes, I do think it should be used here, unless it is deemed to not > fit the pattern/usage model. kref_put() enforces a very specific destructor signature. I know of places where that doesn't work because the destructor takes more than one argument (granted that this is not the case here). So why does kref_put() exist at all? Why not kref_dec_and_test()? Why doesn't refcount_t get merged into kref, or vice versa? Having both would seem redundant. Mind you, I've been gradually reverting atomic_t-to-refcount_t conversions because it seems I'm not allowed refcount_inc/dec_return() and I want to get at the point refcount for tracing purposes. > > > > +module_exit(watch_queue_exit); > > > > > > module_misc_device()? > > > > warthog>git grep module_misc_device -- Documentation/ > > warthog1> > > Do I have to document all helper macros? If you add an API, documenting it is your privilege ;-) It's an important test of the API - if you can't describe it, it's probably wrong. Now I will grant that you didn't add that function... > Anyway, it saves you boilerplate code, but if built in, it's at the module > init level, not the fs init level, like you are asking for here. So that > might not work, it's your call. Actually, I probably shouldn't have a module exit function. It can't be a module as it's called by core code. I'll switch to builtin_misc_device(). > And how does the tracing and perf ring buffers do this without needing > volatile? Why not use the same type of interface they provide, as it's > always good to share code that has already had all of the nasty corner > cases worked out. I've no idea how trace does it - or even where - or even if. As far as I can see, grepping for mmap in kernel/trace/*, there's no mmap support. Reading Documentation/trace/ring-buffer-design.txt the trace subsystem has some sort of transient page fifo which is a lot more complicated than what I want and doesn't look like it'll be mmap'able. Looking at the perf ring buffer, there appears to be a missing barrier in perf_aux_output_end(): rb->user_page->aux_head = rb->aux_head; should be: smp_store_release(&rb->user_page->aux_head, rb->aux_head); It should also be using smp_load_acquire(). See Documentation/core-api/circular-buffers.rst And a (partial) patch has been proposed: https://lkml.org/lkml/2018/5/10/249 David
next prev parent reply other threads:[~2019-05-29 16:06 UTC|newest] Thread overview: 131+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-28 16:01 [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications David Howells 2019-05-28 16:01 ` David Howells 2019-05-28 16:01 ` [PATCH 1/7] General notification queue with user mmap()'able ring buffer David Howells 2019-05-28 16:01 ` David Howells 2019-05-28 16:26 ` Greg KH 2019-05-28 16:26 ` Greg KH 2019-05-28 17:30 ` David Howells 2019-05-28 17:30 ` David Howells 2019-05-28 23:12 ` Greg KH 2019-05-28 23:12 ` Greg KH 2019-05-29 16:06 ` David Howells [this message] 2019-05-29 16:06 ` David Howells 2019-05-29 17:46 ` Jann Horn 2019-05-29 17:46 ` Jann Horn 2019-05-29 21:02 ` David Howells 2019-05-29 21:02 ` David Howells 2019-05-31 11:14 ` Peter Zijlstra 2019-05-31 11:14 ` Peter Zijlstra 2019-05-31 12:02 ` David Howells 2019-05-31 12:02 ` David Howells 2019-05-31 13:26 ` Peter Zijlstra 2019-05-31 13:26 ` Peter Zijlstra 2019-05-31 14:20 ` David Howells 2019-05-31 14:20 ` David Howells 2019-05-31 16:44 ` Peter Zijlstra 2019-05-31 16:44 ` Peter Zijlstra 2019-05-31 17:12 ` David Howells 2019-05-31 17:12 ` David Howells 2019-06-17 16:24 ` Peter Zijlstra 2019-06-17 16:24 ` Peter Zijlstra 2019-05-29 23:09 ` Greg KH 2019-05-29 23:09 ` Greg KH 2019-05-29 23:11 ` Greg KH 2019-05-29 23:11 ` Greg KH 2019-05-30 9:50 ` Andrea Parri 2019-05-30 9:50 ` Andrea Parri 2019-05-31 8:35 ` Peter Zijlstra 2019-05-31 8:35 ` Peter Zijlstra 2019-05-31 8:47 ` Peter Zijlstra 2019-05-31 8:47 ` Peter Zijlstra 2019-05-31 12:42 ` David Howells 2019-05-31 12:42 ` David Howells 2019-05-31 14:55 ` David Howells 2019-05-31 14:55 ` David Howells 2019-05-28 19:14 ` Jann Horn 2019-05-28 19:14 ` Jann Horn 2019-05-28 22:28 ` David Howells 2019-05-28 22:28 ` David Howells 2019-05-28 23:16 ` Jann Horn 2019-05-28 23:16 ` Jann Horn 2019-05-28 16:02 ` [PATCH 2/7] keys: Add a notification facility David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 16:02 ` [PATCH 3/7] vfs: Add a mount-notification facility David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 20:06 ` Jann Horn 2019-05-28 20:06 ` Jann Horn 2019-05-28 23:04 ` David Howells 2019-05-28 23:04 ` David Howells 2019-05-28 23:23 ` Jann Horn 2019-05-28 23:23 ` Jann Horn 2019-05-29 11:16 ` David Howells 2019-05-29 11:16 ` David Howells 2019-05-28 23:08 ` David Howells 2019-05-28 23:08 ` David Howells 2019-05-29 10:55 ` David Howells 2019-05-29 10:55 ` David Howells 2019-05-29 11:00 ` David Howells 2019-05-29 11:00 ` David Howells 2019-05-29 15:53 ` Casey Schaufler 2019-05-29 15:53 ` Casey Schaufler 2019-05-29 16:12 ` Jann Horn 2019-05-29 16:12 ` Jann Horn 2019-05-29 17:04 ` Casey Schaufler 2019-05-29 17:04 ` Casey Schaufler 2019-06-03 16:30 ` David Howells 2019-06-03 16:30 ` David Howells 2019-05-29 17:13 ` Andy Lutomirski 2019-05-29 17:13 ` Andy Lutomirski 2019-05-29 17:46 ` Casey Schaufler 2019-05-29 17:46 ` Casey Schaufler 2019-05-29 18:11 ` Jann Horn 2019-05-29 18:11 ` Jann Horn 2019-05-29 19:28 ` Casey Schaufler 2019-05-29 19:28 ` Casey Schaufler 2019-05-29 19:47 ` Jann Horn 2019-05-29 19:47 ` Jann Horn 2019-05-29 20:50 ` Casey Schaufler 2019-05-29 20:50 ` Casey Schaufler 2019-05-29 23:12 ` Andy Lutomirski 2019-05-29 23:12 ` Andy Lutomirski 2019-05-29 23:56 ` Casey Schaufler 2019-05-29 23:56 ` Casey Schaufler 2019-05-28 16:02 ` [PATCH 4/7] vfs: Add superblock notifications David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 20:27 ` Jann Horn 2019-05-28 20:27 ` Jann Horn 2019-05-29 12:58 ` David Howells 2019-05-29 12:58 ` David Howells 2019-05-29 14:16 ` Jann Horn 2019-05-29 14:16 ` Jann Horn 2019-05-28 16:02 ` [PATCH 5/7] fsinfo: Export superblock notification counter David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 16:02 ` [PATCH 6/7] block: Add block layer notifications David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 20:37 ` Jann Horn 2019-05-28 20:37 ` Jann Horn 2019-05-28 16:02 ` [PATCH 7/7] Add sample notification program David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 23:58 ` [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications Greg KH 2019-05-28 23:58 ` Greg KH 2019-05-29 6:33 ` Amir Goldstein 2019-05-29 6:33 ` Amir Goldstein 2019-05-29 6:33 ` Amir Goldstein 2019-05-29 14:25 ` Jan Kara 2019-05-29 14:25 ` Jan Kara 2019-05-29 15:10 ` Greg KH 2019-05-29 15:10 ` Greg KH 2019-05-29 15:53 ` Amir Goldstein 2019-05-29 15:53 ` Amir Goldstein 2019-05-30 11:00 ` Jan Kara 2019-05-30 11:00 ` Jan Kara 2019-06-04 12:33 ` David Howells 2019-06-04 12:33 ` David Howells 2019-05-29 6:45 ` David Howells 2019-05-29 6:45 ` David Howells 2019-05-29 7:40 ` Amir Goldstein 2019-05-29 7:40 ` Amir Goldstein 2019-05-29 9:09 ` David Howells 2019-05-29 9:09 ` David Howells 2019-05-29 15:41 ` Casey Schaufler 2019-05-29 15:41 ` Casey Schaufler
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=31936.1559146000@warthog.procyon.org.uk \ --to=dhowells@redhat.com \ --cc=gregkh@linuxfoundation.org \ --cc=keyrings@vger.kernel.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-block@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=raven@themaw.net \ --cc=viro@zeniv.linux.org.uk \ /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: linkBe 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.