From: Uladzislau Rezki <urezki@gmail•com>
To: Vlastimil Babka <vbabka@suse•cz>
Cc: "Jason A. Donenfeld" <Jason@zx2c4•com>,
kvm@vger•kernel.org, Neil Brown <neilb@suse•de>,
kernel-janitors@vger•kernel.org,
Olga Kornievskaia <kolga@netapp•com>,
kasan-dev <kasan-dev@googlegroups•com>,
Dai Ngo <Dai.Ngo@oracle•com>,
Christophe Leroy <christophe.leroy@csgroup•eu>,
coreteam@netfilter•org,
"Naveen N. Rao" <naveen.n.rao@linux•ibm.com>,
Jakub Kicinski <kuba@kernel•org>,
linux-trace-kernel@vger•kernel.org, paulmck@kernel•org,
bridge@lists•linux.dev, ecryptfs@vger•kernel.org,
Nicholas Piggin <npiggin@gmail•com>,
linux-can@vger•kernel.org, linux-block@vger•kernel.org,
Mathieu Desnoyers <mathieu.desnoyers@efficios•com>,
Tom Talpey <tom@talpey•com>,
linux-nfs@vger•kernel.org, netdev@vger•kernel.org,
Lai Jiangshan <jiangshanlai@gmail•com>,
linux-kernel@vger•kernel.org,
Julia Lawall <Julia.Lawall@inria•fr>,
Uladzislau Rezki <urezki@gmail•com>,
netfilter-devel@vger•kernel.org, linuxppc-dev@lists•ozlabs.org,
wireguard@lists•zx2c4.com
Subject: Re: [PATCH 00/14] replace call_rcu by kfree_rcu for simple kmem_cache_free callback
Date: Fri, 21 Jun 2024 11:32:12 +0200 [thread overview]
Message-ID: <ZnVInAV8BXhgAjP_@pc636> (raw)
In-Reply-To: <4cba4a48-902b-4fb6-895c-c8e6b64e0d5f@suse.cz>
On Wed, Jun 19, 2024 at 11:28:13AM +0200, Vlastimil Babka wrote:
> On 6/18/24 7:53 PM, Paul E. McKenney wrote:
> > On Tue, Jun 18, 2024 at 07:21:42PM +0200, Vlastimil Babka wrote:
> >> On 6/18/24 6:48 PM, Paul E. McKenney wrote:
> >> > On Tue, Jun 18, 2024 at 11:31:00AM +0200, Uladzislau Rezki wrote:
> >> >> > On 6/17/24 8:42 PM, Uladzislau Rezki wrote:
> >> >> > >> +
> >> >> > >> + s = container_of(work, struct kmem_cache, async_destroy_work);
> >> >> > >> +
> >> >> > >> + // XXX use the real kmem_cache_free_barrier() or similar thing here
> >> >> > > It implies that we need to introduce kfree_rcu_barrier(), a new API, which i
> >> >> > > wanted to avoid initially.
> >> >> >
> >> >> > I wanted to avoid new API or flags for kfree_rcu() users and this would
> >> >> > be achieved. The barrier is used internally so I don't consider that an
> >> >> > API to avoid. How difficult is the implementation is another question,
> >> >> > depending on how the current batching works. Once (if) we have sheaves
> >> >> > proven to work and move kfree_rcu() fully into SLUB, the barrier might
> >> >> > also look different and hopefully easier. So maybe it's not worth to
> >> >> > invest too much into that barrier and just go for the potentially
> >> >> > longer, but easier to implement?
> >> >> >
> >> >> Right. I agree here. If the cache is not empty, OK, we just defer the
> >> >> work, even we can use a big 21 seconds delay, after that we just "warn"
> >> >> if it is still not empty and leave it as it is, i.e. emit a warning and
> >> >> we are done.
> >> >>
> >> >> Destroying the cache is not something that must happen right away.
> >> >
> >> > OK, I have to ask...
> >> >
> >> > Suppose that the cache is created and destroyed by a module and
> >> > init/cleanup time, respectively. Suppose that this module is rmmod'ed
> >> > then very quickly insmod'ed.
> >> >
> >> > Do we need to fail the insmod if the kmem_cache has not yet been fully
> >> > cleaned up?
> >>
> >> We don't have any such link between kmem_cache and module to detect that, so
> >> we would have to start tracking that. Probably not worth the trouble.
> >
> > Fair enough!
> >
> >> > If not, do we have two versions of the same kmem_cache in
> >> > /proc during the overlap time?
> >>
> >> Hm could happen in /proc/slabinfo but without being harmful other than
> >> perhaps confusing someone. We could filter out the caches being destroyed
> >> trivially.
> >
> > Or mark them in /proc/slabinfo? Yet another column, yay!!! Or script
> > breakage from flagging the name somehow, for example, trailing "/"
> > character.
>
> Yeah I've been resisting such changes to the layout and this wouldn't be
> worth it, apart from changing the name itself but not in a dangerous way
> like with "/" :)
>
> >> Sysfs and debugfs might be more problematic as I suppose directory names
> >> would clash. I'll have to check... might be even happening now when we do
> >> detect leaked objects and just leave the cache around... thanks for the
> >> question.
> >
> > "It is a service that I provide." ;-)
> >
> > But yes, we might be living with it already and there might already
> > be ways people deal with it.
>
> So it seems if the sysfs/debugfs directories already exist, they will
> silently not be created. Wonder if we have such cases today already because
> caches with same name exist. I think we do with the zsmalloc using 32 caches
> with same name that we discussed elsewhere just recently.
>
> Also indeed if the cache has leaked objects and won't be thus destroyed,
> these directories indeed stay around, as well as the slabinfo entry, and can
> prevent new ones from being created (slabinfo lines with same name are not
> prevented).
>
> But it wouldn't be great to introduce this possibility to happen for the
> temporarily delayed removal due to kfree_rcu() and a module re-insert, since
> that's a legitimate case and not buggy state due to leaks.
>
> The debugfs directory we could remove immediately before handing over to the
> scheduled workfn, but if it turns out there was a leak and the workfn leaves
> the cache around, debugfs dir will be gone and we can't check the
> alloc_traces/free_traces files there (but we have the per-object info
> including the traces in the dmesg splat).
>
> The sysfs directory is currently removed only with the whole cache being
> destryed due to sysfs/kobject lifetime model. I'd love to untangle it for
> other reasons too, but haven't investigated it yet. But again it might be
> useful for sysfs dir to stay around for inspection, as for the debugfs.
>
> We could rename the sysfs/debugfs directories before queuing the work? Add
> some prefix like GOING_AWAY-$name. If leak is detected and cache stays
> forever, another rename to LEAKED-$name. (and same for the slabinfo). But
> multiple ones with same name might pile up, so try adding a counter then?
> Probably messy to implement, but perhaps the most robust in the end? The
> automatic counter could also solve the general case of people using same
> name for multiple caches.
>
> Other ideas?
>
One question. Maybe it is already late but it is better to ask rather than not.
What do you think if we have a small discussion about it on the LPC 2024 as a
topic? It might be it is already late or a schedule is set by now. Or we fix
it by a conference time.
Just a thought.
--
Uladzislau Rezki
next prev parent reply other threads:[~2024-06-21 9:33 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-09 8:27 [PATCH 00/14] replace call_rcu by kfree_rcu for simple kmem_cache_free callback Julia Lawall
2024-06-09 8:27 ` [PATCH 03/14] KVM: PPC: " Julia Lawall
2024-06-12 21:33 ` [PATCH 00/14] " Jakub Kicinski
2024-06-12 22:37 ` Paul E. McKenney
2024-06-12 22:46 ` Jakub Kicinski
2024-06-12 22:52 ` Jens Axboe
2024-06-12 23:04 ` Paul E. McKenney
2024-06-12 23:31 ` Jason A. Donenfeld
2024-06-13 0:31 ` Jason A. Donenfeld
2024-06-13 3:38 ` Paul E. McKenney
2024-06-13 12:22 ` Jason A. Donenfeld
2024-06-13 12:46 ` Paul E. McKenney
2024-06-13 14:11 ` Jason A. Donenfeld
2024-06-13 15:12 ` Paul E. McKenney
2024-06-17 15:10 ` Vlastimil Babka
2024-06-17 16:12 ` Paul E. McKenney
2024-06-17 17:23 ` Vlastimil Babka
2024-06-17 18:42 ` Uladzislau Rezki
2024-06-17 21:08 ` Vlastimil Babka
2024-06-18 9:31 ` Uladzislau Rezki
2024-06-18 16:48 ` Paul E. McKenney
2024-06-18 17:21 ` Vlastimil Babka
2024-06-18 17:53 ` Paul E. McKenney
2024-06-19 9:28 ` Vlastimil Babka
2024-06-19 16:46 ` Paul E. McKenney
2024-06-21 9:32 ` Uladzislau Rezki [this message]
2024-07-15 20:39 ` Vlastimil Babka
2024-07-24 13:53 ` Paul E. McKenney
2024-07-24 14:40 ` Vlastimil Babka
2024-10-08 16:41 ` Vlastimil Babka
2024-10-08 20:02 ` Paul E. McKenney
2024-10-09 17:08 ` Julia Lawall
2024-10-09 21:02 ` Paul E. McKenney
2024-06-19 9:51 ` Uladzislau Rezki
2024-06-19 9:56 ` Vlastimil Babka
2024-06-19 11:22 ` Uladzislau Rezki
2024-06-17 18:54 ` Paul E. McKenney
2024-06-17 21:34 ` Vlastimil Babka
2024-06-13 14:17 ` Jakub Kicinski
2024-06-13 14:53 ` Paul E. McKenney
2024-06-13 11:58 ` Jason A. Donenfeld
2024-06-13 12:47 ` Paul E. McKenney
2024-06-13 13:06 ` Uladzislau Rezki
2024-06-13 15:06 ` Paul E. McKenney
2024-06-13 17:38 ` Uladzislau Rezki
2024-06-13 17:45 ` Paul E. McKenney
2024-06-13 17:58 ` Uladzislau Rezki
2024-06-13 18:13 ` Paul E. McKenney
2024-06-14 12:35 ` Uladzislau Rezki
2024-06-14 14:17 ` Paul E. McKenney
2024-06-14 14:50 ` Uladzislau Rezki
2024-06-14 19:33 ` Jason A. Donenfeld
2024-06-17 13:50 ` Uladzislau Rezki
2024-06-17 14:56 ` Jason A. Donenfeld
2024-06-17 16:30 ` Uladzislau Rezki
2024-06-17 16:33 ` Jason A. Donenfeld
2024-06-17 16:38 ` Vlastimil Babka
2024-06-17 17:04 ` Jason A. Donenfeld
2024-06-17 21:19 ` Vlastimil Babka
2024-06-17 16:42 ` Uladzislau Rezki
2024-06-17 16:57 ` Jason A. Donenfeld
2024-06-17 17:19 ` Uladzislau Rezki
2024-06-17 14:37 ` Vlastimil Babka
2024-10-08 16:36 ` Vlastimil Babka
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=ZnVInAV8BXhgAjP_@pc636 \
--to=urezki@gmail$(echo .)com \
--cc=Dai.Ngo@oracle$(echo .)com \
--cc=Jason@zx2c4$(echo .)com \
--cc=Julia.Lawall@inria$(echo .)fr \
--cc=bridge@lists$(echo .)linux.dev \
--cc=christophe.leroy@csgroup$(echo .)eu \
--cc=coreteam@netfilter$(echo .)org \
--cc=ecryptfs@vger$(echo .)kernel.org \
--cc=jiangshanlai@gmail$(echo .)com \
--cc=kasan-dev@googlegroups$(echo .)com \
--cc=kernel-janitors@vger$(echo .)kernel.org \
--cc=kolga@netapp$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=kvm@vger$(echo .)kernel.org \
--cc=linux-block@vger$(echo .)kernel.org \
--cc=linux-can@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-nfs@vger$(echo .)kernel.org \
--cc=linux-trace-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mathieu.desnoyers@efficios$(echo .)com \
--cc=naveen.n.rao@linux$(echo .)ibm.com \
--cc=neilb@suse$(echo .)de \
--cc=netdev@vger$(echo .)kernel.org \
--cc=netfilter-devel@vger$(echo .)kernel.org \
--cc=npiggin@gmail$(echo .)com \
--cc=paulmck@kernel$(echo .)org \
--cc=tom@talpey$(echo .)com \
--cc=vbabka@suse$(echo .)cz \
--cc=wireguard@lists$(echo .)zx2c4.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox