From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DD6AEEB64DD for ; Tue, 11 Jul 2023 19:21:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=lyrIXyJpG4yMHcmij917Pna/Dpc30MjCqIVY2diWYW4=; b=3QgVNZ6wiTmKPX DotU22ARav+Ho+RimeIsvXJfixAwhkGc4bKlguWeGZ4sOeGk1vHG2Rdmm+sBhaHGxDdYDqd1RGpFP 9o0lWTwIBadvjYzgjFkeq8nBVo7BoYMm83itfVpU3vXGApesh5RufZLgiwO9fZ9eS3kmjwKs/2fPT ppyE+gdx4GUOEdCsy3E6wFHhXR9BgxsP7KHeUDnCidPIpaLxzRgohfhm1jjKGW5rR44IOv/Fyj7BU z3rT3MUAJZNGF3zTaGwL9yl3V8ch79keHppA13+id8er0675IAKb5gKJIlfxTX+z/zS2H64y0A8/3 4SK+Xscq13QbD6LdgYIg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qJIuY-00Fjgq-2A; Tue, 11 Jul 2023 19:20:34 +0000 Received: from mail-pl1-x62b.google.com ([2607:f8b0:4864:20::62b]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qJIuV-00Fjfe-1u for linux-arm-kernel@lists.infradead.org; Tue, 11 Jul 2023 19:20:33 +0000 Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1b89cfb4571so45518865ad.3 for ; Tue, 11 Jul 2023 12:20:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689103226; x=1691695226; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=f1RdjQ4NrY+dTIP5U7ks2Bp8VWT43m8mQA91HDnFqNQ=; b=dJcHKJ/wkSPqpiz930XCirSfpLMbFYzCQ7DZnephHtgcgDueSJjN0A86ixgq2U0352 3PXeSyaI/fDYieyLcsRROYYRHbFsf4NoXnKEIKHnZrl0QiqbGnPTmfePHjVlUFnHnkgh h5hnJ6h459913WyizTWk+nSfz1gBtp181TKIn3UvHCQjQoj4H5k7OPij+LklYHCClUR2 vUWAHmOL0D8HWUPLcmv1Wb3u7bmV9vQ8rvRRpYPlS5S1Hb4GPiII6RuPSqqTuTjKAyRu Kx9Tl6k2HiDFKjg2aVpSK8JEgLeoGNwtOyAIqYXsVKwph+TFvk5xAaXi+LGRBq1a2zck yr3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689103226; x=1691695226; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=f1RdjQ4NrY+dTIP5U7ks2Bp8VWT43m8mQA91HDnFqNQ=; b=VTxBfCBQKFtOnj/hVye69DQcpAxPtE1LeEl1z5p3HCoOTaZAj8hq+kZHC/zK1vfAlR yj+R29d12UrdNYdwgpioJ6TbOhkHTh4KcqlveKXPeCJYmnUdPF/k5iOe4D1uAtYjgB7L EUffBThsvVrHElG5YCvOGLeTPI/WDu9bNn2GvooaIlZ3Mtb6J6XStdguAeK3deuk5PiM ogOhyunzHTf/s3MvhE5FrlUP3w0yUr7iTpRyEM0KklUn0ab2GXDxnk0KELAvODHN1Aw4 2puDbtj5l0ApkBHDachOdCeMcCJIazHsBcSXh1BYZnZ59/FCNgc6G1O23gjRqz4G32lI HmnQ== X-Gm-Message-State: ABy/qLaYEKoKE4q4i6QU8Tnhx7g6B8eGHCU7Hg3otcfyKosKhbKMufCJ lXUfVtspQKIh6+Ifa8hBWaE= X-Google-Smtp-Source: APBJJlETMmHWv5veKD3fT8ZGjLGUpXn9Wi3gEfg00gV5lXwzh5xAyLs9MkviCWty+0DVsEYqH1bNFQ== X-Received: by 2002:a17:903:1c5:b0:1ac:451d:34a with SMTP id e5-20020a17090301c500b001ac451d034amr19111806plh.33.1689103226404; Tue, 11 Jul 2023 12:20:26 -0700 (PDT) Received: from localhost ([216.228.127.131]) by smtp.gmail.com with ESMTPSA id t8-20020a17090aae0800b00263f40cf83esm8461495pjq.47.2023.07.11.12.20.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Jul 2023 12:20:25 -0700 (PDT) Date: Tue, 11 Jul 2023 12:20:24 -0700 From: Yury Norov To: Alexander Potapenko Cc: catalin.marinas@arm.com, will@kernel.org, pcc@google.com, andreyknvl@gmail.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, eugenis@google.com, Andy Shevchenko , Rasmus Villemoes Subject: Re: [Resend v1 1/5] linux/bitqueue.h: add the bit queue implementation Message-ID: References: <20230711144233.3129207-1-glider@google.com> <20230711144233.3129207-2-glider@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230711144233.3129207-2-glider@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230711_122031_628920_C3A74FA0 X-CRM114-Status: GOOD ( 38.21 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org + Andy and Rasmus On Tue, Jul 11, 2023 at 04:42:29PM +0200, Alexander Potapenko wrote: > struct bitq represents a bit queue with external storage. > > Its purpose is to easily pack sub-byte values, which can be used, for > example, to implement RLE algorithms. Whatever it is, it's not a queue. The queue implies O(1) for insertion and deletion, but your 'dequeue' is clearly an O(n) procedure. I'm not sure if I completely understand the purpose of the series, but from this description: enqueueing/dequeueing of sub-byte values I think, the simplest solution would be a circular queue (ringbuffer) based on bitmaps: Init an empty 10-bit bitmap: Head = 0 v ..... ..... ^ Tail = 1 Enqueue 6-bit value 0b101010 at the end: Head = 0 v 10101 0.... ^ Tail = 1 Dequeue 3 bits from the beginning: Head = 0 v ...01 0.... ^ Tail = 1 And so on... See some other comments inline. Thanks, Yury > Signed-off-by: Alexander Potapenko > --- > include/linux/bitqueue.h | 144 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 144 insertions(+) > create mode 100644 include/linux/bitqueue.h > > diff --git a/include/linux/bitqueue.h b/include/linux/bitqueue.h > new file mode 100644 > index 0000000000000..c4393f703c697 > --- /dev/null > +++ b/include/linux/bitqueue.h > @@ -0,0 +1,144 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * A simple bit queue which supports enqueueing/dequeueing of sub-byte values. > + * > + * This can be used to pack complex bitfields into byte arrays. > + */ > +#ifndef _LINUX_BITQUEUE_H > +#define _LINUX_BITQUEUE_H > + > +#include > +#include > + > +/** > + * struct bitq - represents a bit queue with external storage. > + * @data: data buffer used by the queue. > + * @size: size of @data in bytes. > + * @bit_pos: current bit position. > + */ > +struct bitq { > + u8 *data; > + int size, bit_pos; > +}; > + > +/** > + * bitq_init - initialize an empty bit queue. > + * @q: struct bitq to be initialized. > + * @data: external data buffer to use. > + * @size: capacity in bytes. > + * > + * Return: 0 in the case of success, -1 if either of the pointers is NULL. ENIVAL? > + */ > +static inline int bitq_init(struct bitq *q, u8 *data, int size) > +{ > + if (!q || !data) > + return -1; This is a useless check. Erroneous code may (and often does) pass a broken pointer other than NULL. And to make it worse, this error handling (instead of run-time segfault which can be easily caught at debugging) may make users think that passing NULL is the right thing. Check bit/bitmap and other kernel functions: they don't check against NULL as a general rule, except for well-justified cases, like 'free(NULL)'. > + q->data = data; > + q->size = size; > + memset(data, 0, size); Useless memset? > + q->bit_pos = 0; > + return 0; > +} > + > +/** > + * bitq_init_full - make a bit queue from an initialized byte array. > + * @q: struct bitq to be initialized. > + * @data: external data buffer to use. > + * @size: capacity in bytes. > + * > + * Return: 0 in the case of success, -1 if either of the pointers is NULL. > + */ > +static inline int bitq_init_full(struct bitq *q, u8 *data, int size) > +{ > + if (!q || !data) > + return -1; > + q->data = data; > + q->size = size; > + q->bit_pos = q->size * 8; > + return 0; > +} This all should not reside in a header. > + > +/** > + * bitq_enqueue - push up to 8 bits to the end of the queue. > + * @q: struct bitq. > + * @value: byte containing the value to be pushed. > + * @bits: number of bits (1 to 8) to push. > + * > + * Return: number of bits pushed, or -1 in the case of an error. > + */ > +static inline int bitq_enqueue(struct bitq *q, u8 value, int bits) > +{ > + int byte_pos, left_in_byte, max_pos; > + u8 hi, lo; > + > + if (!q || (bits < 1) || (bits > 8)) > + return -1; Pushing 0 elements in queue is usually not an error. Implementations usually return and do nothing. From the malloc() man page: If size is 0, then malloc() returns a unique pointer value that can later be successfully passed to free(). > + max_pos = q->size * 8; > + if ((max_pos - q->bit_pos) < bits) > + return -1; ENOMEM? Or probably better to resize the queue. > + > + left_in_byte = 8 - (q->bit_pos % 8); > + byte_pos = q->bit_pos / 8; > + /* Clamp @value. */ > + value %= (1 << bits); > + if (left_in_byte >= bits) { > + /* @value fits into the current byte. */ > + value <<= (left_in_byte - bits); > + q->data[byte_pos] |= value; > + } else { > + /* > + * @value needs to be split between the current and the > + * following bytes. > + */ > + hi = value >> (bits - left_in_byte); > + q->data[byte_pos] |= hi; > + byte_pos++; > + lo = value << (8 - (bits - left_in_byte)); > + q->data[byte_pos] |= lo; > + } This piece should be a bitmap_append() function, like: bitmap_append(addr, 3, 2, 0b11) would append 0b11 to the bitmap at offset 3. We already have bitmap_{set,get}_value8, so I suggest to extend the interface for unaligned offsets and lengths up to BITS_PER_LONG. > + q->bit_pos += bits; > + return bits; > +} > + > +/** > + * bitq_dequeue - pop up to 8 bits from the beginning of the queue. > + * @q: struct bitq. > + * @value: u8* to store the popped value (can be NULL). > + * @bits: number of bits (1 to 8) to pop. > + * > + * Return: number of bits popped, or -1 in the case of an error. > + */ > + > +#include > +static inline int bitq_dequeue(struct bitq *q, u8 *value, int bits) > +{ > + int rem_bits = 8 - bits, i; > + u8 output; > + > + /* Invalid arguments. */ > + if (!q || (bits < 1) || (bits > 8)) > + return -1; > + /* Not enough space to insert @bits. */ > + if (q->bit_pos < bits) > + return -1; > + /* Take the first @bits bits from the first byte. */ > + output = q->data[0]; > + output >>= rem_bits; > + if (value) > + *value = output; > + > + /* > + * Shift every byte in the queue to the left by @bits, carrying over to > + * the previous byte. > + */ > + for (i = 0; i < q->size - 1; i++) { > + q->data[i] = (q->data[i] << bits) | > + (q->data[i + 1] >> rem_bits); > + } As I already mentioned, this is O(N), which is wrong for queues. Add a pointer to the head in the bitq structure to avoid shifting every byte. BTW, we've got bitmap_shift_{left,right} for this. > + q->data[q->size - 1] <<= bits; > + q->bit_pos -= bits; > + return bits; > +} > + > +#endif // _LINUX_BITQUEUE_H > -- > 2.41.0.255.g8b1d071c50-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel