From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 083143DB635 for ; Mon, 15 Jun 2026 09:56:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781517420; cv=none; b=MQtmZrOwYM0TFsjQ8qQTAF7xXZAbrwaQ7kcobgTa3c74qHnSsRXr6iLcw2x0B37MXcM1+MiMt+TFHWJzyBOR9sAtYdnXT1k1lRP9tIzHAHD9M247Koj0gp309u3m0hEnwjdx8roToh9JQSEJ5sD4q96BA4iJ4IEPlNrsHgRqQzo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781517420; c=relaxed/simple; bh=lDPDEQ5fI45IRASsYo8Nlab/XqpFz9ivprB73riS4kA=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=R+vjnkDvYt/guXkN0ENKp2F9vlnnbIfCqzxnszMfpBhTDY0l930bzzeSGTJEvEMZ8WNyjsIPqWaxdnsT6B07PZ8zOjxlHZzS6v6h5/1JKhc/7ER7PZa5FGxPrVqmHmTyXxdwonuzXiTyeLBSByMLO8YB2+fTSIli1/xl5HXoWk8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=iv+3qFZ/; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=j02vhVaZ; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="iv+3qFZ/"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="j02vhVaZ" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1781517417; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=hdVZ8Zm6R4iX8++LAiMGKRGC+5n+aNHI7iGhJHzDLTU=; b=iv+3qFZ/xU90TCqYEBk0SKD2fbMC13w4CW7Be7dgbUTzE+r9c7qLEJX4HmmHPXsjvkRL6E xENhRITDtG7bNkyijD6yU+VRmngCPOcWUezwofN/eoXhRSPfZ46rWlKTHb1P+qzY9Go4Iu wh8UFmzeUq8e/gZ6qhX6PyqP3PxHTJA= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-632-WMgwn0_lOsa9Na5Iqs_4Rw-1; Mon, 15 Jun 2026 05:56:56 -0400 X-MC-Unique: WMgwn0_lOsa9Na5Iqs_4Rw-1 X-Mimecast-MFC-AGG-ID: WMgwn0_lOsa9Na5Iqs_4Rw_1781517415 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-49221de4ed4so11690175e9.0 for ; Mon, 15 Jun 2026 02:56:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1781517415; x=1782122215; darn=vger.kernel.org; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject :date:message-id:reply-to; bh=hdVZ8Zm6R4iX8++LAiMGKRGC+5n+aNHI7iGhJHzDLTU=; b=j02vhVaZ1ATdWhdT1JUzDnOgMx1nnLfxNh+JitVv+U16Hq+f48m+K4+QtxuhobO3zD wbDVAjpPsdOkGHyXARZt7UoINY2Cqx5QQs6oUHQs3ruhfHuK+Gte2kr3n3qq00lKRexw 8eAFDn2XkTr81YITlaZec6cSqxL1P6EieNqjOF4fZbIci/+G9/r5NAWFS9cOoz6Jf5W6 clWZZU5i7KxdTNtJ8+4DkS3jS6UjO/SNKyJhV3+p0hruNIMmdmlz9RxG5kL1QQ8oL4Jt c0RpP4bAZI/NJ7PSF4BKq4WrhcT/g2WH4KGJ7jmaHrIV6meOyDuDsaBOjXAOzkIm0EjZ Ieyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781517415; x=1782122215; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=hdVZ8Zm6R4iX8++LAiMGKRGC+5n+aNHI7iGhJHzDLTU=; b=WKgmPXKbATHTiqnBZ2e5n6HEFr7UStB46q4wgWngNPUwoJvGmr6EmT/9X5pLNN2z6B uogR4Eldcmdsv0Ntgh3XU1/+dEzvS1hz6dg25pU0vXjLJyaiCSwogv6nrYE7Y88PTMTC 97jQ3c8UiWah4k7cm9FpJ4+p+4ik0DW+jdUXO3N4BumgO5851WBulCLc91JJQ4Hg7gyA LvFaB2P1ibHLTttTxRpzOEVjG+n4LFwvVj6vYor+kc2aQSRPXYErmyZI9Twsqw2lX3i4 hgXa1C4/jgQVAYcGYlVWGNjPN2K9JwuQg9d4JbqVmB4R5BsohkAkhJcZ+kr1T7ei6+Ob QCkw== X-Forwarded-Encrypted: i=1; AFNElJ8joPvEQ06f+/fANebxVk9hyjuH6ynkdpJr4XWG89D5y7Tn9IvdPRvGAa/m6pxmESqEJvl58pV7fGCJFwQ=@vger.kernel.org X-Gm-Message-State: AOJu0YwjQYmZ78XuhEjyah/SKi00VY38ttG+f/SYBnVy9feuX5f4sCTX mHzAy9PwqTnK4uqZh6R4ojrO4hh9X61F3o79TAvW6gxSlhI7IqNsZPExK+4uJRgPF3Q05/Kk2qE iY+CbN0HlheckgFjIEyTX8tSw8lQ2QkkAp2LnvEI49awEoZaWmSuf/NhMm/kKLh8hJA== X-Gm-Gg: Acq92OGbgi01qIw3OIsJfN1t6mxFYYyK0xaULYGKCRx2XdYXimaRH5I2kZptmv/kfiF Dqv9LIwwwNnlPYPAZvYcNBsryPnjVOP2f9VvaI4Qvy/8xdY99y/lvqa0YYEIk9EwOyva+bh0Eza CgaMdIs9uxGnsDqqwo3d4dGgUF9Jxo0WAJ2grRjKfn6Dm7ggBSZsHodZ8c+HAyo7G2RFimCv16Q ErRcNZAw3cq3erwN1J31hfjjbSjdqtOvqEgdvAOhDRaaMPi87qm76lS4gG+PVaYy8V7XtWVnw+A Sm1enurmRnLih4nMxEgcXbrN9GdLHw5cVwRh7KQMTRqNDY5pXicDW72k4HigeXN5Hor2PIil++Q gsBttzyzAh3FCAGxJgRzH0uDRAw== X-Received: by 2002:a05:600c:5394:b0:492:29a1:98c4 with SMTP id 5b1f17b1804b1-49229a1991emr51447775e9.8.1781517415026; Mon, 15 Jun 2026 02:56:55 -0700 (PDT) X-Received: by 2002:a05:600c:5394:b0:492:29a1:98c4 with SMTP id 5b1f17b1804b1-49229a1991emr51447505e9.8.1781517414624; Mon, 15 Jun 2026 02:56:54 -0700 (PDT) Received: from [192.168.1.167] ([185.168.96.228]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-490ea95c51dsm227714945e9.1.2026.06.15.02.56.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 Jun 2026 02:56:54 -0700 (PDT) Message-ID: Subject: Re: [PATCH v3 1/9] rv/da: introduce DA_MON_ALLOCATION_STRATEGY From: Gabriele Monaco To: wen.yang@linux.dev Cc: Steven Rostedt , linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org Date: Mon, 15 Jun 2026 11:56:53 +0200 In-Reply-To: <496394879a590b4d7bafdb2f13618d2e30be982f.1780847473.git.wen.yang@linux.dev> References: <496394879a590b4d7bafdb2f13618d2e30be982f.1780847473.git.wen.yang@linux.dev> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.60.2 (3.60.2-1.fc44) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Mon, 2026-06-08 at 00:13 +0800, wen.yang@linux.dev wrote: > +#ifndef DA_MON_ALLOCATION_STRATEGY > +# define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_AUTO > +#endif I'm not sure the space goes there according to kernel coding style, we don't use it in this file and clang-format removes it. I'd keep consistency. ... > =C2=A0 > +/* > + * DA_MON_POOL_SIZE must be defined before this header is included I don't think we need to be this verbose (also, ha_monitor may not even be included if that's a da_monitor). I would stop at the line above. > (directly or > + * transitively via ha_monitor.h) when DA_ALLOC_POOL is selected.=C2=A0 > In practice > + * this means defining it after the monitor's model header (which > supplies the > + * capacity constant) and before the ha_monitor.h include. > + */ > +#if DA_MON_ALLOCATION_STRATEGY =3D=3D DA_ALLOC_POOL && > !defined(DA_MON_POOL_SIZE) > +# error "DA_ALLOC_POOL requires DA_MON_POOL_SIZE to be defined > before including this header" Same here I'd keep consistency and remove the space before error. > +#endif ... =C2=A0 > +/* > + * Per-object pool state. > + * > + * Zero-initialised by default (storage =3D=3D NULL =E2=9F=B9 kmalloc mo= de).=C2=A0 A Mmh, =E2=9F=B9 doesn't seem to print that well on my terminal, let's perha= ps use plain old ASCII =3D> . Also I don't find what you put in parentheses to be adding much value, we could even omit it. I remember discussing about this so I may have missed your answer, but why don't we handle this pool as a simple kmem_cache/mempool instead of implementing a similar logic from scratch? > monitor > + * opts into pool mode by defining DA_MON_ALLOCATION_STRATEGY > DA_ALLOC_POOL > + * and DA_MON_POOL_SIZE before including this header; > da_monitor_init() then > + * pre-allocates the pool internally. > + * > + * Because every field is wrapped in this struct and the struct > itself is a > + * per-TU static, each monitor that includes this header gets a > completely > + * independent pool.=C2=A0 A kmalloc monitor (e.g. nomiss) and a pool > monitor > + * (e.g. tlob) therefore coexist without any interference. > + * > + * da_pool_return_cb runs from softirq (non-PREEMPT_RT) or rcuc > kthread > + * (PREEMPT_RT); spin_lock_irqsave handles both. > + */ > +struct da_per_obj_pool { > + struct da_monitor_storage=C2=A0 *storage;=C2=A0 /* non-NULL =E2=9F=B9 p= ool > mode */ > + struct da_monitor_storage **free;=C2=A0=C2=A0=C2=A0=C2=A0 /* kmalloc'd = pointer > stack */ > + unsigned int=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 free_top; > + unsigned int=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 capacity; /* total number of > slots */ > + spinlock_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 lock; > +}; > + > +static struct da_per_obj_pool da_pool =3D { > + .lock =3D __SPIN_LOCK_UNLOCKED(da_pool.lock), > +}; ... > =C2=A0/* > =C2=A0 * da_destroy_storage - destroy the per-object storage > =C2=A0 * > - * The caller is responsible to synchronise writers, either with > locks or > - * implicitly. For instance, if da_destroy_storage is called at > sched_exit and > - * da_create_storage can never occur after that, it's safe to call > this without > - * locks. > - * This function includes an RCU read-side critical section to > synchronise > - * against da_monitor_destroy(). > + * Pool mode: removes from hash and returns the slot via call_rcu(). > + * Kmalloc mode: removes from hash and frees via kfree_rcu(). > + * > + * Includes an RCU read-side critical section to synchronise against > + * da_monitor_destroy(). > =C2=A0 */ > =C2=A0static inline void da_destroy_storage(da_id_type id) > =C2=A0{ > @@ -558,7 +670,11 @@ static inline void da_destroy_storage(da_id_type > id) > =C2=A0 return; > =C2=A0 da_monitor_reset_hook(&mon_storage->rv.da_mon); > =C2=A0 hash_del_rcu(&mon_storage->node); > +#if DA_MON_ALLOCATION_STRATEGY =3D=3D DA_ALLOC_POOL > + call_rcu(&mon_storage->rcu, da_pool_return_cb); > +#else ifdeffery in functions is discouraged as quite unreadable. Since DA_MON_ALLOCATION_STRATEGY is guaranteed to be defined, simple C ifs are going to be mostly equivalent (the compiler will cut instead of the preprocessor, but still). > =C2=A0 kfree_rcu(mon_storage, rcu); > +#endif > =C2=A0} ... > + > +/* > + * da_monitor_init - initialise the per-object monitor > + * > + * Selects the allocation path at compile time based on > DA_MON_ALLOCATION_STRATEGY: > + *=C2=A0=C2=A0 DA_ALLOC_POOL=C2=A0=C2=A0 - pre-allocates DA_MON_POOL_SIZ= E storage slots. > + *=C2=A0=C2=A0 DA_ALLOC_AUTO / DA_ALLOC_MANUAL - initialises the hash ta= ble > only. > + */ > =C2=A0static inline int da_monitor_init(void) > =C2=A0{ > =C2=A0 hash_init(da_monitor_ht); > +#if DA_MON_ALLOCATION_STRATEGY =3D=3D DA_ALLOC_POOL > + return __da_monitor_init_pool(DA_MON_POOL_SIZE); > +#else Same here, use if() > =C2=A0 return 0; > +#endif > =C2=A0} > =C2=A0 > -static inline void da_monitor_destroy(void) > +static inline void da_monitor_destroy_pool(void) > +{ > + struct da_monitor_storage *ms; > + struct hlist_node *tmp; > + int bkt; > + > + /* > + * Ensure all in-flight tracepoint handlers that may hold a > raw pointer > + * to a pool slot (e.g. tlob_stop_task after its RCU guard > exits) have > + * completed before we begin tearing down the pool.=C2=A0 Mirrors > the same > + * call in da_monitor_destroy_kmalloc(). > + */ > + tracepoint_synchronize_unregister(); > + This is common between the pool and kmalloc flavours, you can leave it in da_monitor_destroy() before branching. > + /* > + * Drain any entries that were not stopped before destroy > (e.g. > + * uprobe-started sessions whose stop probe never fired).=C2=A0 > Call > + * da_extra_cleanup() before hash_del_rcu() so the hook may > safely > + * call ha_cancel_timer_sync() while the monitor is still > reachable. > + */ > + hash_for_each_safe(da_monitor_ht, bkt, tmp, ms, node) { > + da_extra_cleanup(&ms->rv.da_mon); > + hash_del_rcu(&ms->node); > + call_rcu(&ms->rcu, da_pool_return_cb); > + } Cannot you make also this common? da_extra_cleanup() should be called in all flavours and the only difference I see here is the rcu callback. Also do you really need call_rcu() ? Since we should already have waited for a grace period, you can probably call the function directly. If not, I'd still try and make both flavours consistent (sync + free OR call_rcu + barrier, not both). > + > + /* > + * rcu_barrier() drains every pending call_rcu() callback, > including > + * both da_pool_return_cb() and any monitor-specific free > callbacks > + * (e.g. tlob_free_rcu) enqueued by da_extra_cleanup(). > + */ > + rcu_barrier(); > + kfree(da_pool.storage); > + da_pool.storage =3D NULL; > + kfree(da_pool.free); > + da_pool.free =3D NULL; > + da_pool.free_top =3D 0; > + da_pool.capacity =3D 0; Only this part is really specific to this allocation flavour, if you want it in a separate function, go ahead, but the rest should probably share as much code as possible (especially the cleanup/synchronisation mess we just worked out). Thanks, Gabriele