Received-SPF: pass (google.com: domain of linux-kernel+bounces-83398-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 27 Feb 2024 15:27:42 +0100
From: Benjamin Tissoires <bentiss@kernel.org>
To: Eduard Zingerman <eddyz87@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>, 
	Daniel Borkmann <daniel@iogearbox.net>, John Fastabend <john.fastabend@gmail.com>, 
	Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, 
	Song Liu <song@kernel.org>, Yonghong Song <yonghong.song@linux.dev>, 
	KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, 
	Jiri Olsa <jolsa@kernel.org>, Jiri Kosina <jikos@kernel.org>, 
	Benjamin Tissoires <benjamin.tissoires@redhat.com>, Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>, 
	bpf@vger.kernel.org, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, 
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH RFC bpf-next v3 04/16] bpf/helpers: introduce sleepable
 bpf_timers
Message-ID: <3jtqytxfqymfx2fenqby2x3zzra63tj7jrxrmunqsk6l7dqyip@jt7kdhxeb4np>
References: <20240221-hid-bpf-sleepable-v3-0-1fb378ca6301@kernel.org>
 <20240221-hid-bpf-sleepable-v3-4-1fb378ca6301@kernel.org>
 <818e43447651af1a659993897c14d05fec5038e4.camel@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <818e43447651af1a659993897c14d05fec5038e4.camel@gmail.com>

On Feb 23 2024, Eduard Zingerman wrote:
> On Wed, 2024-02-21 at 17:25 +0100, Benjamin Tissoires wrote:
> 
> [...]
> 
> > @@ -1282,7 +1333,7 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
> >  
> >  	if (in_nmi())
> >  		return -EOPNOTSUPP;
> > -	if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN))
> > +	if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN | BPF_F_TIMER_SLEEPABLE))
> >  		return -EINVAL;
> >  	__bpf_spin_lock_irqsave(&timer->lock);
> >  	t = timer->timer;
> > @@ -1299,7 +1350,10 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
> >  	if (flags & BPF_F_TIMER_CPU_PIN)
> >  		mode |= HRTIMER_MODE_PINNED;
> >  
> > -	hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
> > +	if (flags & BPF_F_TIMER_SLEEPABLE)
> > +		schedule_work(&t->work);
> > +	else
> > +		hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
> 
> It looks like nsecs is simply ignored for sleepable timers.
> Should this be hrtimer_start() that waits nsecs and schedules work,
> or schedule_delayed_work()? (but it takes delay in jiffies, which is
> probably too coarse). Sorry if I miss something.

Yeah, I agree it's confusing, but as mentioned by Toke in his reply, we
should return -EINVAL if a timer value is provided (for now).

Alexei mentioned[0] that he didn't want to mix delays in hrtimers with
workqueue as they are non deterministic. So AFAIU, I should add the only
guarantee we can provide: a sleepable context, and proper delays in
sleepable contexts will be added once we have a better workqueue
selection available.

Cheers,
Benjamin

[0] https://lore.kernel.org/bpf/CAO-hwJKz+eRA+BFLANTrEqz2jQAOANTE3c7eqNJ6wDqJR7jMiQ@mail.gmail.com/T/#md15e431cbcddec9fcaddf1c305234523ed26f7ce