Re: [PATCH v1 1/5] relayfs: support a counter tracking if per-cpu buffers is full
From: Andrew Morton <akpm@linux-foundation.org>
Date: 2025-05-13 00:51:48
Also in:
linux-block, lkml
On Mon, 12 May 2025 10:49:31 +0800 Jason Xing [off-list ref] wrote:
From: Jason Xing <kernelxing@tencent.com> Add 'full' field in per-cpu buffer structure to detect if the buffer is full, which means: 1) relayfs doesn't intend to accept new data in non-overwrite mode that is also by default, or 2) relayfs is going to start over again and overwrite old unread data when kernel module has its own subbuf_start callback to support overwrite mode. This counter works for both overwrite and non-overwrite modes. This counter doesn't have any explicit lock to protect from being modified by different threads at the same time for the better performance consideration. In terms of the atomic operation, it's not introduced for incrementing the counter like blktrace because side effect may arise when multiple threads access the counter simultaneously on the machine equipped with many cpus, say, more than 200. As we can see in relay_write() and __relay_write(), the writer at the beginning should consider how to use the lock for the whole write process, thus it's not necessary to add another lock to make sure the counter is accurate. Using 'pahole --hex -C rchan_buf vmlinux' so you can see this field just fits into one 4-byte hole in the cacheline 2.
Does this alter blktrace output? If so is that backward-compatible (and do we care). Is there any blktrace documentation which should be updated? Also, please check Documentation/filesystems/relay.rst and see if any updates should be made to reflect the changes in this patchset. I'm not really clear on the use cases of this counter - perhaps you can be more verbose about this in the changelog.
quoted hunk ↗ jump to hunk
diff --git a/include/linux/relay.h b/include/linux/relay.h index f80b0eb1e905..022cf11e5a92 100644 --- a/include/linux/relay.h +++ b/include/linux/relay.h@@ -28,6 +28,14 @@ */ #define RELAYFS_CHANNEL_VERSION 7 +/* + * Relay buffer error statistics dump + */ +struct rchan_buf_error_stats +{ + unsigned int full; /* counter for buffer full */ +};
Why a struct?
quoted hunk ↗ jump to hunk
/* * Per-cpu relay channel buffer */@@ -43,6 +51,7 @@ struct rchan_buf struct irq_work wakeup_work; /* reader wakeup */ struct dentry *dentry; /* channel file dentry */ struct kref kref; /* channel buffer refcount */ + struct rchan_buf_error_stats stats; /* error stats */
Could simply use unsigned int full; here? Also, the name "full" implies to me "it is full". Perhaps "full_count" would be better.
quoted hunk ↗ jump to hunk
struct page **page_array; /* array of current buffer pages */ unsigned int page_count; /* number of current buffer pages */ unsigned int finalized; /* buffer has been finalized */diff --git a/kernel/relay.c b/kernel/relay.c index 5aeb9226e238..b5db4aa60da1 100644 --- a/kernel/relay.c +++ b/kernel/relay.c@@ -252,8 +252,13 @@ EXPORT_SYMBOL_GPL(relay_buf_full); static int relay_subbuf_start(struct rchan_buf *buf, void *subbuf, void *prev_subbuf) { + int buf_full = relay_buf_full(buf); + + if (buf_full) + buf->stats.full++;
I don't understand the changelog's description of this, sorry. Is it saying "this operation is protected by a lock"? If so, please specifically state which lock this is. Or is it saying "we don't care if this races because the counter will be close enough". If so then maybe so, but things like KCSAN will probably detect and warn and then people will want to address this.