From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com [209.85.208.53]) (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 DD948194AE8; Fri, 11 Oct 2024 15:44:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.53 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728661442; cv=none; b=vArPoV1yOTxVXbaGFuKf9X9SJxgvVAdkIBUivDuRFCaGTZxH7eop8yvqhSgKB6fG47JNwczssNvpSq0C1M1W3orAIMYgH0uux5JTd1t7AZkuQ1K5dQkJI/b+7sDXdQ3cJgRGVxbzXo50TK3B7eCUFKjJn/sX7J9DnKCL0tz281w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728661442; c=relaxed/simple; bh=PooPGWvgmo1UR9R92pPvRf5+4zgiaZSygxrcEWt+qUQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=JvcfSxto1seTmUSNWIr1IPClqixItdPtLtgpTFj1KgglopEF0/AtY26DGhn4v3pMK3f111LVrGpE58sg6drvSYUYMOLb3EjAyjtek6tau6IErDSeM+Ko/EjciYX2v+ZwZjEqjSfZFBsLFM0so0CFgveKjbwsRlzIvyFSsIX11CI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=YEX+jkfU; arc=none smtp.client-ip=209.85.208.53 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="YEX+jkfU" Received: by mail-ed1-f53.google.com with SMTP id 4fb4d7f45d1cf-5c942815197so961902a12.1; Fri, 11 Oct 2024 08:44:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1728661439; x=1729266239; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=OrxxF3yNL/neU0KeLnUooiqCBgYtc6nVU7hAyqRA2W0=; b=YEX+jkfUEToiS4Yfq3TI15EgGV38mDqbL6quudSJStG0trg7ZC4e6L/UtNDBrHDESt D/mQoPycT/aBarZIKIhwjsavVxHgMUR/qilOWBNhHZsJP47iaix0uyd2hzjGfmATYqWK LCdZx/h1MKZYPXpnPCtE4+s+CGp68UWylKU1NQI0ywdXOKPneyXbkFxwJbP5mT2SDbKY h6e2fouLrvKUSikKhnY9BaPonu51DfbxoqSBEg/bMAnSrnV1vfJrJo/oMPhLU7O5blsb KuRqJGPK+xdjKAI7wZ/mm/dtF2ep2WreSmv7fpjf1lNwYyljSLSVYWirZpuq6aSNgEJL UOcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728661439; x=1729266239; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=OrxxF3yNL/neU0KeLnUooiqCBgYtc6nVU7hAyqRA2W0=; b=GwXfFkFrmOq62FbevCOYz5Snteo6Xz78Nk+cNXaT638dNyVADMRtQf2PT1HF3Jm665 TC3QzrWiw5Jcl8i/8Efaqcfc8fFOMO0Yt967ePQOjBc6rS1DCJwl01aDGAlAauHyADPl +3X4ft6FJRuVYIz/saBli4zPRuNU+//qRO34kwC60nEM6lEXmjF7QZqflJzRFIZDxa9W 8Omwk/OpcT8WGKMFz+pC4Uefdgf4/ugaUYtKq+eWMgXwUwHNPNJN51l8fmZ0ZEgG+utH gXx949qgUq3Gs3usiYSqcwprr+V/iYeA/FJ19tERflA5OBc1Xns0GV/vagyo683hbiKE hYZg== X-Forwarded-Encrypted: i=1; AJvYcCUhGfvFac9ZLv612k8+yoNG/d2Kd7Y3rUcyLX9Ww/d2EUJUcSp71D45SpHmcciNEJ6w+ByUH3hJidJLPK458D4=@vger.kernel.org, AJvYcCVnKb4uJkirOWiW+qMPVEXls8CMl1X7nE7BP5vACK+SggK+p6y9ZUVTCFTmI2t2UbXwJ8Z4gAgC5WGyQbE=@vger.kernel.org X-Gm-Message-State: AOJu0YzvsERrNu+0FG39ByJh60fxMEnsUhW/Ukp8/ibjnR7u1mwS0i/j cYWNbwhsh1Sm9H6QDEsDuZ1+pz308EVxP6Ig+O1ue6EALerPpqWr X-Google-Smtp-Source: AGHT+IFY98Tc3cVGdGaOgsDFu7925UW/KVti8l3p26KH+Ng8egwPJ2H13W2ZO7izpyk7+n7RsmcfQg== X-Received: by 2002:a05:6402:4410:b0:5c8:7bbd:1c4f with SMTP id 4fb4d7f45d1cf-5c948c882fdmr3297711a12.7.1728661438938; Fri, 11 Oct 2024 08:43:58 -0700 (PDT) Received: from ?IPV6:2003:df:bf0d:b400:a17a:b60f:897e:1773? (p200300dfbf0db400a17ab60f897e1773.dip0.t-ipconnect.de. [2003:df:bf0d:b400:a17a:b60f:897e:1773]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5c937153939sm2049178a12.47.2024.10.11.08.43.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 11 Oct 2024 08:43:58 -0700 (PDT) Message-ID: Date: Fri, 11 Oct 2024 17:43:57 +0200 Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 00/14] hrtimer Rust API To: Andreas Hindborg , Boqun Feng , Lyude Paul Cc: Dirk Behme , Miguel Ojeda , Alex Gaynor , Anna-Maria Behnsen , Frederic Weisbecker , Thomas Gleixner , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Alice Ryhl , rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org References: <20240917222739.1298275-1-a.hindborg@kernel.org> <87a5falmjy.fsf@kernel.org> Content-Language: de-AT-frami From: Dirk Behme In-Reply-To: <87a5falmjy.fsf@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi Andreas, Am 11.10.24 um 16:52 schrieb Andreas Hindborg: > > Dirk, thanks for reporting! :) > Boqun Feng writes: > >> On Tue, Oct 01, 2024 at 02:37:46PM +0200, Dirk Behme wrote: >>> On 18.09.2024 00:27, Andreas Hindborg wrote: >>>> Hi! >>>> >>>> This series adds support for using the `hrtimer` subsystem from Rust code. >>>> >>>> I tried breaking up the code in some smaller patches, hopefully that will >>>> ease the review process a bit. >>> >>> Just fyi, having all 14 patches applied I get [1] on the first (doctest) >>> Example from hrtimer.rs. >>> >>> This is from lockdep: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/locking/lockdep.c#n4785 >>> >>> Having just a quick look I'm not sure what the root cause is. Maybe mutex in >>> interrupt context? Or a more subtle one? >> >> I think it's calling mutex inside an interrupt context as shown by the >> callstack: >> >> ] __mutex_lock+0xa0/0xa4 >> ] ... >> ] hrtimer_interrupt+0x1d4/0x2ac >> >> , it is because: >> >> +//! struct ArcIntrusiveTimer { >> +//! #[pin] >> +//! timer: Timer, >> +//! #[pin] >> +//! flag: Mutex, >> +//! #[pin] >> +//! cond: CondVar, >> +//! } >> >> has a Mutex, which actually should be a SpinLockIrq [1]. Note that >> irq-off is needed for the lock, because otherwise we will hit a self >> deadlock due to interrupts: >> >> spin_lock(&a); >> > timer interrupt >> spin_lock(&a); >> >> Also notice that the IrqDisabled<'_> token can be simply created by >> ::new(), because irq contexts should guarantee interrupt disabled (i.e. >> we don't support nested interrupts*). > > I updated the example based on the work in [1]. I think we need to > update `CondVar::wait` to support waiting with irq disabled. Yes, I agree. This answers one of the open questions I had in the discussion with Boqun :) What do you think regarding the other open question: In this *special* case here, what do you think to go *without* any lock? I mean the 'while *guard != 5' loop in the main thread is read only regarding guard. So it doesn't matter if it *reads* the old or the new value. And the read/modify/write of guard in the callback is done with interrupts disabled anyhow as it runs in interrupt context. And with this can't be interrupted (excluding nested interrupts). So this modification of guard doesn't need to be protected from being interrupted by a lock if there is no modifcation of guard "outside" the interupt locked context. What do you think? Thanks Dirk > Without > this, when we get back from `bindings::schedule_timeout` in > `CondVar::wait_internal`, interrupts are enabled: > > ```rust > use kernel::{ > hrtimer::{Timer, TimerCallback, TimerPointer, TimerRestart}, > impl_has_timer, new_condvar, new_spinlock, new_spinlock_irq, > irq::IrqDisabled, > prelude::*, > sync::{Arc, ArcBorrow, CondVar, SpinLock, SpinLockIrq}, > time::Ktime, > }; > > #[pin_data] > struct ArcIntrusiveTimer { > #[pin] > timer: Timer, > #[pin] > flag: SpinLockIrq, > #[pin] > cond: CondVar, > } > > impl ArcIntrusiveTimer { > fn new() -> impl PinInit { > try_pin_init!(Self { > timer <- Timer::new(), > flag <- new_spinlock_irq!(0), > cond <- new_condvar!(), > }) > } > } > > impl TimerCallback for ArcIntrusiveTimer { > type CallbackTarget<'a> = Arc; > type CallbackTargetParameter<'a> = ArcBorrow<'a, Self>; > > fn run(this: Self::CallbackTargetParameter<'_>, irq: IrqDisabled<'_>) -> TimerRestart { > pr_info!("Timer called\n"); > let mut guard = this.flag.lock_with(irq); > *guard += 1; > this.cond.notify_all(); > if *guard == 5 { > TimerRestart::NoRestart > } > else { > TimerRestart::Restart > > } > } > } > > impl_has_timer! { > impl HasTimer for ArcIntrusiveTimer { self.timer } > } > > > let has_timer = Arc::pin_init(ArcIntrusiveTimer::new(), GFP_KERNEL)?; > let _handle = has_timer.clone().schedule(Ktime::from_ns(200_000_000)); > > kernel::irq::with_irqs_disabled(|irq| { > let mut guard = has_timer.flag.lock_with(irq); > > while *guard != 5 { > pr_info!("Not 5 yet, waiting\n"); > has_timer.cond.wait(&mut guard); // <-- we arrive back here with interrupts enabled! > } > }); > ``` > > I think an update of `CondVar::wait` should be part of the patch set [1]. > > > Best regards, > Andreas > > > [1] https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/ > >