On Wed, Jun 2, 2021 at 7:04 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Jun 02, 2021 at 03:34:29PM -0700, Andrii Nakryiko wrote:
> >
> > > /* copy everything but bpf_spin_lock */
> > > static inline void copy_map_value(struct bpf_map *map, void *dst, void *src)
> > > {
> > > + u32 off = 0, size = 0;
> > > +
> > > if (unlikely(map_value_has_spin_lock(map))) {
> > > - u32 off = map->spin_lock_off;
> > > + off = map->spin_lock_off;
> > > + size = sizeof(struct bpf_spin_lock);
> > > + } else if (unlikely(map_value_has_timer(map))) {
> > > + off = map->timer_off;
> > > + size = sizeof(struct bpf_timer);
> > > + }
> >
> > so the need to handle 0, 1, or 2 gaps seems to be the only reason to
> > disallow both bpf_spinlock and bpf_timer in one map element, right?
>
> exactly.
>
> > Isn't it worth addressing it from the very beginning to lift the
> > artificial restriction? E.g., for speed, you'd do:
> >
> > if (likely(neither spinlock nor timer)) {
> > /* fastest pass */
> > } else if (only one of spinlock or timer) {
> > /* do what you do here */
> > } else {
> > int off1, off2, sz1, sz2;
> >
> > if (spinlock_off < timer_off) {
> > off1 = spinlock_off;
> > sz1 = spinlock_sz;
> > off2 = timer_off;
> > sz2 = timer_sz;
> > } else {
> > ... you get the idea
>
> Not really :)
hm, really? I meant that else will be:
off1 = timer_off;
sz1 = timer_sz;
off2 = spinlock_off;
sz2 = spinlock_sz;
Just making sure that off1 < off2 always and sz1 and sz2 are matching
> Are you suggesting to support one bpf_spin_lock and one
> bpf_timer inside single map element, but not two spin_locks
> and/or not two bpf_timers?
Yes, exactly. I see bpf_spinlock and bpf_timer as two independent
orthogonal features and I don't understand why we restrict using just
one of them in a given map element. I think those 20 lines of code
that decouples them and removes artificial restriction that users need
to remember (or discover with surprise) is totally worth it.
> Two me it's either one or support any.
I think it's fine to start with supporting one. But one of each. They
are independent of each other.
> Anything in-between doesn't seem worth extra code.
Up to you, but I disagree, obviously. It's possible to work-around
that limitation with extra maps/complexity, so if I ever need to both
lock an element and schedule the timer with it, it's not going to stop
me. :)
>
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index f386f85aee5c..0a828dc4968e 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -3241,6 +3241,15 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
> > > return -EACCES;
> > > }
> > > }
> > > + if (map_value_has_timer(map)) {
> > > + u32 t = map->timer_off;
> > > +
> > > + if (reg->smin_value + off < t + sizeof(struct bpf_timer) &&
> >
> > <= ? Otherwise we allow accessing the first byte, unless I'm mistaken.
>
> I don't think so. See the comment above in if (map_value_has_spin_lock(map))
> I didn't copy-paste it, because it's the same logic.
Oh, I didn't realize that this is the interval intersection check I
suggested a long time ago :) yeah, that still looks correct
>
> > > - if (val) {
> > > - /* todo: relax this requirement */
> > > - verbose(env, "bpf_timer field can only be first in the map value element\n");
> >
> > ok, this was confusing, but now I see why you did that...
>
> I'll clarify the comment to say that the next patch fixes it.
ok, thanks
