From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) (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 3DC212FDC27 for ; Fri, 26 Jun 2026 23:28:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.171 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782516521; cv=none; b=kvS1tvyXgeLgRGgILmZJyXTDk52mUbu5MwtIeQIWJdRfaoo7QphW+Jo5Ent9LqrZija18bqNz3tesxJvYjhfFkko4qF5fVXvUmfLm1w046mIK9r1Zu8gp1pS23MrYx/TIpVsmsdc04iERHLGvUHOn+XyuDNYi5OuAj4+BBJ2+0c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782516521; c=relaxed/simple; bh=g/9XI3ior4FGSepU4aTRl5CoHf1XN5AXCFZXSsNzAp0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=b/BcoLP7GyfIde2lPO/kp5M9L/IuwGOfghnieo3E144K2hKB2EaF/j5RpPHDZiNIPx6YHtbfOH14Mp1qOzwoVfBPPbHVWuA0RwI4c9NH5M2OV+eAwEToNRhlPILsYh7himUSCssr1sSo1Wc0NEGxdEHFrKVjibl3FIfegcgFYjQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=A258+glQ; arc=none smtp.client-ip=209.85.214.171 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=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="A258+glQ" Received: by mail-pl1-f171.google.com with SMTP id d9443c01a7336-2c9b2ac97cdso255ad.1 for ; Fri, 26 Jun 2026 16:28:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1782516519; x=1783121319; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=d743/IMP0R5FGHdDnybTt/easCOX5xZy3RcR2x9mo8k=; b=A258+glQ/cWY/fESE+mSbn/yyw2epMhvN3GnvhPhIQPN25fo5Gf7mqGFG6iHHwnWWP D4mOYSVNJiXgWPlQcNtbh1rXuQuTobYmyHdxZA/2aLXT7bUWYq+3Dlf0LMmpsWUhDR+X trd17tox8Hp7LYYMQ2MUSXAB63zPXBH2Z/uKKEz8rtchX6ioGhi1JG7wm+MOq0yfDfQZ ZAA0m5qMCOWdVqwDF+a/FVe0BBPw1NxUEMQOqfBBnZuU2RwUx4BaX3Kdqnx88CwKKcAK nIIzXX1B6QxF9PHLChVtT5I1J76FegqcnHkgIFuYXsqFMmrcjaiC7x7EUkartMsFc6vD 4H1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1782516519; x=1783121319; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=d743/IMP0R5FGHdDnybTt/easCOX5xZy3RcR2x9mo8k=; b=DU/mvv5EDYsQlnS803Vqo+LoeIu8wU1k+6aB21paeTcTrdO7ALRt3Vd5/8lfsJuEhf bMFMfi9L13mZVuhA0ix7TdLMHwyQxIu+yN4WqB69RlqNmIUZ3nNISDGoGmw/DHXNHasK FLENz5sQlB/K046tytwpVusqfilFk3iedF/yTwLpZnSf3HqHkmHFR523x2b0ss6W+YOs 0QJxfnUSIKL+1PEjtlalquOHpSQ9bBmUZPlRsyQVuDQRHTdJMDpUzaR+d+pjuwAfGYv1 USYX/VbGaI4bxTJL6aro7iirt8Zkc64AgMlA9PPuVBFAaOdPDULB101U0E8o1Qk7fmVe bu5w== X-Forwarded-Encrypted: i=1; AHgh+RoYq23VlNNT+CX/MRFsJuGRIDJHHTpziMf6IYOirlAcPRqSFP35NkF8gZcGoxQZ88GfVElZqpxjSZhwjxM=@vger.kernel.org X-Gm-Message-State: AOJu0Yx52XIwbajnDTiE2sY5UOkp/lxfJjc46Uj2TVAlETP/gDuKDdSx 5d6Mz+5Lf3rDK+LpFoEOa8jxcaeHhsCu4OcWKDxyCBEO30RBaXmXsHlKt6LlfBkK2g== X-Gm-Gg: AfdE7cmyUo+KDU58FhcYOrXc6PIFdO26cYvPnC8YkbQYfDbf0WSvrHU0VhxAhpoo88f tnnydjMB83Jx2AAi5ICG5CnPLaOh7766nG5hK9ndgPdsp3yH8XwHxlHXfK25/m9CBPCnJFW5bkc ITpaamWb8dN33cmoD8F0YGBAgMdj8hY9bZ3HnIcdLVniZTgl0ruBLIJSZ5rkiuiSjzTVjA23s5g VglnD4JIO+PcgjUcjcFdvbsd6i6A9N+h76CYgwB+JGVBdHxjbIX1gxSIzm1sFmuQ29xIITRDPKw Uf6+faeBaOJ8iBEta96VgMnAv+DOhEEPGy/jchzkEcyNWjoMXa+7CiEh0nrnucmMKT5yhIwRO/+ bKnI/DWvCteItR37k700KXaXa7qGOunVfJ+HQGSkC9v3ZpPQWkvIp+qw581w+X2Nc0g/zec9ZXR cV+vpTtc/mbJjnHcLKfOT7efW9exFuvuZPACm4JWc5/jE5fESD9zAG1xhvInElGdJ2Uce4iFvSC g8nmGLbjtO3ICS+gVQu2eoTYLDvVfYXT5ZfKjDiZTpueQ== X-Received: by 2002:a17:903:2acc:b0:2c7:f6ca:2deb with SMTP id d9443c01a7336-2c9a5f976e6mr865865ad.22.1782516518994; Fri, 26 Jun 2026 16:28:38 -0700 (PDT) Received: from google.com (112.174.16.34.bc.googleusercontent.com. [34.16.174.112]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2c7f5ac9285sm47910125ad.11.2026.06.26.16.28.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Jun 2026 16:28:38 -0700 (PDT) Date: Fri, 26 Jun 2026 23:28:34 +0000 From: Carlos Llamas To: Alice Ryhl Cc: Greg Kroah-Hartman , Boqun Feng , Gary Guo , Onur =?iso-8859-1?Q?=D6zkan?= , Andreas Hindborg , Benno Lossin , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Daniel Almeida , Danilo Krummrich , Ingo Molnar , Lyude Paul , Miguel Ojeda , Peter Zijlstra , Trevor Gross , Waiman Long , Will Deacon , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH 4/5] rust_binder: consolidate transaction failure prints Message-ID: References: <20260623-pr-ratelimited-v1-0-cc922f544dc0@google.com> <20260623-pr-ratelimited-v1-4-cc922f544dc0@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260623-pr-ratelimited-v1-4-cc922f544dc0@google.com> On Tue, Jun 23, 2026 at 03:38:07PM +0000, Alice Ryhl wrote: > When a transaction fails, it currently hits multiple pr_warn! > invocations meaning that a single failure can result in several lines in > the kernel log. This is unnecessary, so consolidate them into one print > used for all transaction failures. > > For ENOSPC errors we want to know the transaction size, so a field is > added to TransactionInfo that bubbles up this information to the new > print location. > > Signed-off-by: Alice Ryhl > --- > drivers/android/binder/error.rs | 4 --- > drivers/android/binder/thread.rs | 59 ++++++++++++++++++++--------------- > drivers/android/binder/transaction.rs | 21 +++---------- > rust/kernel/error.rs | 2 +- > 4 files changed, 39 insertions(+), 47 deletions(-) > > diff --git a/drivers/android/binder/error.rs b/drivers/android/binder/error.rs > index 1296072c35d9..aed1c747640b 100644 > --- a/drivers/android/binder/error.rs > +++ b/drivers/android/binder/error.rs > @@ -37,10 +37,6 @@ pub(crate) fn new_frozen_oneway() -> Self { > source: None, > } > } > - > - pub(crate) fn is_dead(&self) -> bool { > - self.reply == BR_DEAD_REPLY > - } > } > > /// Convert an errno into a `BinderError` and store the errno used to construct it. The errno > diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs > index def00edce9c4..a5b038877747 100644 > --- a/drivers/android/binder/thread.rs > +++ b/drivers/android/binder/thread.rs > @@ -25,7 +25,7 @@ > use crate::{ > allocation::{Allocation, AllocationView, BinderObject, BinderObjectRef, NewAllocation}, > defs::*, > - error::BinderResult, > + error::{BinderError, BinderResult}, > process::{GetWorkOrRegister, Process}, > ptr_align, > stats::GLOBAL_STATS, > @@ -1018,18 +1018,9 @@ pub(crate) fn copy_transaction_data( > .ok_or(ENOMEM)?, > size_of::(), > ); > + info.debug_total_size = len; > let secctx_off = aligned_data_size + offsets_size + buffers_size; > - let mut alloc = match to_process.buffer_alloc(debug_id, len, info) { > - Ok(alloc) => alloc, > - Err(err) => { > - pr_warn!( > - "Failed to allocate buffer. len:{}, is_oneway:{}", > - len, > - info.is_oneway(), > - ); > - return Err(err); > - } > - }; > + let mut alloc = to_process.buffer_alloc(debug_id, len, info)?; > > let mut buffer_reader = UserSlice::new(info.data_ptr, data_size).reader(); > let mut end_of_previous_object = 0; > @@ -1259,6 +1250,15 @@ fn read_transaction_info( > > info.debug_id = super::next_debug_id(); > > + // We don't yet know the real message size because it depends on the secctx size, so for > + // now write an estimate. When the real value is computed, this estimate is replaced. > + // Writing an estimate here ensures that the approx message size can still be printed if > + // the transaction fails before it is computed exactly. > + info.debug_total_size = info > + .data_size > + .saturating_add(info.offsets_size) > + .saturating_add(info.buffers_size); hmm, I'm not so sure about this "estimation". I can see how it might be confusing against the "final" size, after secctx and alignment, bla bla. I wonder if it would be best to leave this as zero (until this is known) or maybe have the "raw" individual values from userspace? > + > Ok(()) > } > > @@ -1277,6 +1277,9 @@ fn transaction(self: &Arc, cmd: u32, reader: &mut UserSliceReader) -> Resu > self.transaction_inner(&mut info) > }; > > + // This runs when return work is passed to the caller. This is not > + // always the same as the transaction failing, as reply errors are > + // delivered to the remote process. > if let Err(err) = ret { > self.push_return_work(err.reply); > if err.reply != BR_TRANSACTION_COMPLETE { > @@ -1290,13 +1293,6 @@ fn transaction(self: &Arc, cmd: u32, reader: &mut UserSliceReader) -> Resu > ExtendedError::new(info.debug_id as u32, err.reply, source.to_errno()); > } > } > - > - pr_warn!( > - "{}:{} transaction to {} failed: {err:?}", > - info.from_pid, > - info.from_tid, > - info.to_pid > - ); > } > } > > @@ -1305,8 +1301,27 @@ fn transaction(self: &Arc, cmd: u32, reader: &mut UserSliceReader) -> Resu > // useful in case the transaction failed with BR_TRANSACTION_PENDING_FROZEN. > info.report_netlink(BR_ONEWAY_SPAM_SUSPECT, &self.process.ctx); > } > + // This runs when the transaction failed. > if info.reply != 0 { > info.report_netlink(info.reply, &self.process.ctx); > + let err = BinderError { > + reply: info.reply, > + source: Error::try_from_errno(info.errno), > + }; > + pr_warn!( > + "{}:{} {} to {} failed: {err:?}, {} bytes\n", > + info.from_pid, > + info.from_tid, > + if info.is_reply { > + "reply" > + } else if info.is_oneway() { > + "oneway" > + } else { > + "transaction" > + }, > + info.to_pid, > + info.debug_total_size > + ); > } > > Ok(()) > @@ -1374,12 +1389,6 @@ fn reply_inner(self: &Arc, info: &mut TransactionInfo) -> BinderResult { > // At this point we only return `BR_TRANSACTION_COMPLETE` to the caller, and we must let > // the sender know that the transaction has completed (with an error in this case). > > - pr_warn!( > - "{}:{} reply to {} failed: {err:?}", > - info.from_pid, > - info.from_tid, > - info.to_pid > - ); > let param = err.source.as_ref().map_or(0, |e| e.to_errno()); > let ee = ExtendedError::new(info.debug_id as u32, err.reply, param); > orig.from > diff --git a/drivers/android/binder/transaction.rs b/drivers/android/binder/transaction.rs > index 9aefa01599fb..316627e5f9ac 100644 > --- a/drivers/android/binder/transaction.rs > +++ b/drivers/android/binder/transaction.rs > @@ -39,6 +39,7 @@ pub(crate) struct TransactionInfo { > pub(crate) offsets_ptr: UserPtr, > pub(crate) offsets_size: usize, > pub(crate) buffers_size: usize, > + pub(crate) debug_total_size: usize, > pub(crate) target_handle: u32, > pub(crate) errno: i32, > pub(crate) reply: u32, > @@ -138,21 +139,13 @@ pub(crate) fn new( > let txn_security_ctx = node_ref.node.flags & FLAT_BINDER_FLAG_TXN_SECURITY_CTX != 0; > let mut txn_security_ctx_off = if txn_security_ctx { Some(0) } else { None }; > let to = node_ref.node.owner.clone(); > - let mut alloc = match from.copy_transaction_data( > + let mut alloc = from.copy_transaction_data( > to.clone(), > info, > info.debug_id, > allow_fds, > txn_security_ctx_off.as_mut(), > - ) { > - Ok(alloc) => alloc, > - Err(err) => { > - if !err.is_dead() { > - pr_warn!("Failure in copy_transaction_data: {:?}", err); > - } > - return Err(err); > - } > - }; > + )?; > if info.is_oneway() { > if from_parent.is_some() { > pr_warn!("Oneway transaction should not be in a transaction stack."); > @@ -193,13 +186,7 @@ pub(crate) fn new_reply( > allow_fds: bool, > ) -> BinderResult> { > let mut alloc = > - match from.copy_transaction_data(to.clone(), info, info.debug_id, allow_fds, None) { > - Ok(alloc) => alloc, > - Err(err) => { > - pr_warn!("Failure in copy_transaction_data: {:?}", err); > - return Err(err); > - } > - }; > + from.copy_transaction_data(to.clone(), info, info.debug_id, allow_fds, None)?; > if info.flags & TF_CLEAR_BUF != 0 { > alloc.set_info_clear_on_drop(); > } > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs > index 05cf869ac090..89c247f150b3 100644 > --- a/rust/kernel/error.rs > +++ b/rust/kernel/error.rs > @@ -137,7 +137,7 @@ pub fn from_errno(errno: crate::ffi::c_int) -> Error { > /// Creates an [`Error`] from a kernel error code. > /// > /// Returns [`None`] if `errno` is out-of-range. > - const fn try_from_errno(errno: crate::ffi::c_int) -> Option { > + pub const fn try_from_errno(errno: crate::ffi::c_int) -> Option { > if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 { > return None; > } > > -- > 2.55.0.rc0.799.gd6f94ed593-goog >