Re: [RFC PATCH 18/20] ceph: add comments to metadata structures in string_table.h

[Max Kellermann <max@blarg.de>]

On 2025/09/05 22:01, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> +/*
> + * Reference-counted string metadata: Interned string with automatic memory
> + * management and deduplication. Uses red-black tree for efficient lookup and
> + * RCU for safe concurrent access. Strings are immutable and shared across
> + * multiple users to reduce memory usage.
> + */

No, it doesn't use rbtree AND rcu.  It uses rbtree OR rcu - it's a
union, it cannot use both.  But when does it use one or the other?
That would have been helpful to know.

>  struct ceph_string {
> +	/* Reference counting for automatic cleanup */
>  	struct kref kref;

In the other patch, you wrote that reference counting is "for safe
shared access", and now it's "for automatic cleanup".  But it's really
neither.  Reference counters do not do automatic cleanup.  They are a
tool to be able to detect when the last reference is dropped.  And
then you can do the cleanup.  But that is not automatic.  You have to
implement it manually for eac use of struct kref.

This comment is confusing and adds no value.

>  	union {
> +		/* Red-black tree node for string table lookup */
>  		struct rb_node node;
> +		/* RCU head for safe deferred cleanup */
>  		struct rcu_head rcu;

These two comments add no value, they don't explain anything that
isn't already obvious from looking at the struct types.  Explaining
that "rb_node" is a "Red-black tree node" is just noise that doesn't
belong here; if anything, it belongs to the rbtree_node documentation.
Repeating such text everywhere has a negative impact on people's time.

>  	};
> +	/* Length of the string in bytes */
>  	size_t len;
> +	/* Variable-length string data (NUL-terminated) */
>  	char str[];

The "[]" and the "NUL-terminated" and the "len" field already imply
that it's variable-length.  Writing this again is just noise.