From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f74.google.com (mail-wm1-f74.google.com [209.85.128.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D3559F9CB for ; Tue, 11 Mar 2025 10:07:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741687640; cv=none; b=kR+8+IhqTgi2IRMasbK49AtcFH+94LK0KLLTSs/hHsru7szAoCE/B5L8p0df2twvtR0LfaLDCSSdQCz0U/1vxkeRlApc2RnlFA3QbOep7wq3oLsanIhox6Up/eWSfOfuvyFYcJp3IldsEh2DrOfi4GERi1QQ0rII3UIzSDuBypA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741687640; c=relaxed/simple; bh=ks/m4Dg9e0LsX5W+24SIh9BwJqlTaUJ+OARn0HBP7Yw=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=oOwq4p+vFt4mcLM2YRSLG4xfp+MfBzgv9mIPMhyLB3agqYL/V88/oaCU5B5LP7nIDgChjG+6GrSYBirDp9WNv0PQ2aE6MViH0sRTWVelEGEyDOdsxHbD7Mjn9xjLa6P/rwPlP5MLHLhpJBOqjCCSwqP0we4VwRvqJnLqMkEKe7A= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=vHMgUjq/; arc=none smtp.client-ip=209.85.128.74 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="vHMgUjq/" Received: by mail-wm1-f74.google.com with SMTP id 5b1f17b1804b1-43bc97e6360so26101215e9.3 for ; Tue, 11 Mar 2025 03:07:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1741687637; x=1742292437; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=6gKIm9f9CvLqznwxBzrfnGnuGny9X8dBk6FhNmJ3B9Q=; b=vHMgUjq/ssU1zIaLt1vOipk+B7sHPXhE17iQFWczzaFqVJ/j0wqhc5KnqkAae2mooI wcHM9Wmf2GmY+Z/tFvDvkdmwGVf6Vep7qNyimmyZokE6QyNDaGGjk2Xbrc3OUzXagWOh X5TIbKEW0jZIwucLSAjxkqBf0zeIVI2r5aVD9tLpNZW2E74Tyv14p21nWwUbmMEQJkpd RE1WZmGSWfHulrSI2nEmOUo2devSWHQDr+Lo6E9o/G1R+LN6SXHutCKLgqPbinxcoVm1 DKVMR1GtO/yS8RRoDsdsYfoa6LSQy6CHKh2H7IwAMP82Uk6nVIC3dd4Yn+uORzAbEetv G+jw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741687637; x=1742292437; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=6gKIm9f9CvLqznwxBzrfnGnuGny9X8dBk6FhNmJ3B9Q=; b=pjhOyA8lhoBqLqofMp5TauW1sw5mIQHGuCLsmbnSTorFmuL0P9m9sBt94Zg+znsdzb ebyhXxtT5hjaOj39Vxw8q76X6GWa7k5PnchM9gubvDRf65uiVQikXHL+F8Uqp85EJced t/sk3/teAkS/zec7weHP8vDxLELwSrsAlt1/yQXV/0YC5WYpiu3NrAVepJCeiEd4Guyv +nKHcOPf6mC7N7rzq8wdQFm/IzzHB1tlJz3jvzipaiPZw530Z5TyRTY+waoAjf3LrylR gODaX/IGlbmZYdWFmF3Xbtbgw9OrRx3IxQ2OFP0pfzCUgOE4ltHsJwy8quFgit+vfTcf leCQ== X-Forwarded-Encrypted: i=1; AJvYcCXU/CGi1Wk95leBUedZb4XjYf8/X4xfgdHUcZ9TCAvfE11rh7iG0qK6RJDuOiBajYbd5zhxZfhoEMt6XNLt/g==@vger.kernel.org X-Gm-Message-State: AOJu0YxCWQGHW8EnHwJxI7OAc5qo4vYfT+Qiz6GfZN1SOvRY3IBelCfg AdGwXhLoypyQweVxPhVTnNTw1i990Gbcvz7W+4ThGk+YOudhzxwOtTYluZRCeXvUciC75NrcdY6 pXd+jsIqMQWBxDw== X-Google-Smtp-Source: AGHT+IHm3IFgyjHliXGRSyHcjqv5KSX/xVFX6JhRSVcGMyAOulpQHr5iOaqTDRsQTZV+1DDQGv83dxho+R9CegY= X-Received: from wmgg5.prod.google.com ([2002:a05:600d:5:b0:43b:bd03:5d2]) (user=aliceryhl job=prod-delivery.src-stubby-dispatcher) by 2002:a05:600c:5120:b0:43c:f85d:1245 with SMTP id 5b1f17b1804b1-43cf85d1427mr68893195e9.17.1741687637311; Tue, 11 Mar 2025 03:07:17 -0700 (PDT) Date: Tue, 11 Mar 2025 10:07:15 +0000 In-Reply-To: Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20250310161947.1767855-2-bqe@google.com> Message-ID: Subject: Re: [PATCH v3] rust: add bindings and API for bitmap.h and bitops.h. From: Alice Ryhl To: Yury Norov Cc: Burak Emir , Rasmus Villemoes , Viresh Kumar , Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , "=?utf-8?B?QmrDtnJu?= Roy Baron" , Benno Lossin , Andreas Hindborg , Trevor Gross , rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" On Mon, Mar 10, 2025 at 02:12:22PM -0400, Yury Norov wrote: > On Mon, Mar 10, 2025 at 04:19:46PM +0000, Burak Emir wrote: > > Adds a Rust bitmap API and necessary bitmap and bitops bindings. > > These are for porting the approach from commit 15d9da3f818c ("binder: > > use bitmap for faster descriptor lookup") to Rust. The functionality > > in dbitmap.h makes use of bitmap and bitops. > > Please add it in the same series that converts dbitmap to rust. This > all is a dead code otherwise, right? Rust Binder is not upstream yet. We are upstreaming its dependencies before the driver itself, so there will be dead code no matter what. The number of dependencies is so large that it's completely impractical to land them together with the driver. We can include a patch that includes the wrapper data structure that Rust Binder builds on top of bitmap. We can also include a link to the Rust Binder change that makes Binder start using this code. > > + let ptr = unsafe { bindings::bitmap_zalloc(nbits_u32, flags.as_raw()) }; > > + // Zero-size allocation is ok and yields a dangling pointer. > > Zero-sized allocation makes no sense, and usually is a sign of a bug. > What for you explicitly allow it? I do think that it makes sense to allow a bitmap of size zero. We allow bitmaps of any other length. Why should that length be special? Of course, I guess it might make sense to not call `bitmap_zalloc` when calling `new(0)`? But kmalloc does seem to allow them. > > + /// Copies all bits from `src` and sets any remaining bits to zero. > > + /// > > + /// # Panics > > + /// > > + /// Panics if `src.nbits` has more bits than this bitmap. > > + #[inline] > > + pub fn copy_from_bitmap_and_extend(&mut self, src: &Bitmap) { > > + if self.nbits < src.nbits { > > + panic_not_in_bounds_le("src.nbits", self.nbits, src.nbits); > > The _lt usually stands for 'less than', or '<'. And _le is 'less than or > equal', or '<='. But in your code you do exactly opposite. Is that on > purpose? > > Also, you can make it similar to BUG_ON() semantics, so that it will > be a single line of code, not 3: > > RUST_PANIC("Copy: out of bonds", self.nbits < src.nbits); > > And to that extend, panic message should be available to all rust > subsystems, just like BUG_ON(). We could use assert!(src.nbits <= self.nbits, "Copy: out of bounds."); but using these explicit function calls would generate less code and avoid duplicating the error messages. Also, we should add #[track_caller] to these methods so that the line number in the panic message is taken from the caller rather than this file. > > + } > > + } > > + > > + /// Finds the next zero bit, searching up to `nbits` bits, with offset `offset`. > > + /// > > + /// # Panics > > + /// > > + /// Panics if `nbits` is too large for this bitmap. > > + #[inline] > > + pub fn find_next_zero_bit_upto_offset(&self, nbits: usize, offset: usize) -> usize { > > + if self.nbits < nbits { > > + panic_not_in_bounds_le("nbits", self.nbits, nbits); > > + } > > + // SAFETY: nbits == 0 and out-of-bounds offset is supported, and access is within bounds. > > find_bit() functions are all safe against nbits == 0 or > offset >= nbits. If you add those panics for hardening reasons - it's > OK. If you add them to make your code safer - you don't need them. The > C version is already safe. Ah, that's nice! I do think it's still good to have them for hardening reasons. Passing an out-of-bouds offset is a bug. Alice