| Message ID | 20210112194143.1494-4-yuri.benditovich@daynix.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support for virtio-net hash reporting | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Tue, Jan 12, 2021 at 11:42 AM Yuri Benditovich <yuri.benditovich@daynix.com> wrote: > > This program type can set skb hash value. It will be useful > when the tun will support hash reporting feature if virtio-net. > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > --- > drivers/net/tun.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 7959b5c2d11f..455f7afc1f36 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > prog = NULL; > } else { > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > + if (IS_ERR(prog)) > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); You've ignored the feedback and just resend? what for?
On Tue, Jan 12, 2021 at 9:46 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jan 12, 2021 at 11:42 AM Yuri Benditovich > <yuri.benditovich@daynix.com> wrote: > > > > This program type can set skb hash value. It will be useful > > when the tun will support hash reporting feature if virtio-net. > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > --- > > drivers/net/tun.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index 7959b5c2d11f..455f7afc1f36 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > > prog = NULL; > > } else { > > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > > + if (IS_ERR(prog)) > > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); > > You've ignored the feedback and just resend? what for? No, I do not. Some patches did not reach relevant people at all, so I just resent _all_ the patches to all the people. I will copy your earlier comment to this patch and will address it in the discussion. Sorry for misunderstanding and some redundant noise.
On Tue, Jan 12, 2021 at 9:42 PM Yuri Benditovich <yuri.benditovich@daynix.com> wrote: > > This program type can set skb hash value. It will be useful > when the tun will support hash reporting feature if virtio-net. > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > --- > drivers/net/tun.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 7959b5c2d11f..455f7afc1f36 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > prog = NULL; > } else { > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > + if (IS_ERR(prog)) > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); > if (IS_ERR(prog)) > return PTR_ERR(prog); > } Comment from Alexei Starovoitov: Patches 1 and 2 are missing for me, so I couldn't review properly, but this diff looks odd. It allows sched_cls prog type to attach to tun. That means everything that sched_cls progs can do will be done from tun hook? sched_cls assumes l2 and can modify the packet. I think crashes are inevitable. > -- > 2.17.1 >
On Tue, Jan 12, 2021 at 10:40 PM Yuri Benditovich <yuri.benditovich@daynix.com> wrote: > > On Tue, Jan 12, 2021 at 9:42 PM Yuri Benditovich > <yuri.benditovich@daynix.com> wrote: > > > > This program type can set skb hash value. It will be useful > > when the tun will support hash reporting feature if virtio-net. > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > --- > > drivers/net/tun.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > index 7959b5c2d11f..455f7afc1f36 100644 > > --- a/drivers/net/tun.c > > +++ b/drivers/net/tun.c > > @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > > prog = NULL; > > } else { > > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > > + if (IS_ERR(prog)) > > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); > > if (IS_ERR(prog)) > > return PTR_ERR(prog); > > } > > Comment from Alexei Starovoitov: > Patches 1 and 2 are missing for me, so I couldn't review properly, > but this diff looks odd. > It allows sched_cls prog type to attach to tun. > That means everything that sched_cls progs can do will be done from tun hook? We do not have an intention to modify the packet in this steering eBPF. There is just one function that unavailable for BPF_PROG_TYPE_SOCKET_FILTER that the eBPF needs to make possible to deliver the hash to the guest VM - it is 'bpf_set_hash' Does it mean that we need to define a new eBPF type for socket filter operations + set_hash? Our problem is that the eBPF calculates 32-bit hash, 16-bit queue index and 8-bit of hash type. But it is able to return only 32-bit integer, so in this set of patches the eBPF returns queue index and hash type and saves the hash in skb->hash using bpf_set_hash(). If this is unacceptable, can you please recommend a better solution? > sched_cls assumes l2 and can modify the packet. The steering eBPF in TUN module also assumes l2. > I think crashes are inevitable. > > > -- > > 2.17.1 > >
Hello Alexei, Can you please answer the questions in the last email of this thread? Your comment will be extremely helpful. Thanks On Tue, Jan 12, 2021 at 10:55 PM Yuri Benditovich <yuri.benditovich@daynix.com> wrote: > > On Tue, Jan 12, 2021 at 10:40 PM Yuri Benditovich > <yuri.benditovich@daynix.com> wrote: > > > > On Tue, Jan 12, 2021 at 9:42 PM Yuri Benditovich > > <yuri.benditovich@daynix.com> wrote: > > > > > > This program type can set skb hash value. It will be useful > > > when the tun will support hash reporting feature if virtio-net. > > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > > --- > > > drivers/net/tun.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > index 7959b5c2d11f..455f7afc1f36 100644 > > > --- a/drivers/net/tun.c > > > +++ b/drivers/net/tun.c > > > @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > > > prog = NULL; > > > } else { > > > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > > > + if (IS_ERR(prog)) > > > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); > > > if (IS_ERR(prog)) > > > return PTR_ERR(prog); > > > } > > > > Comment from Alexei Starovoitov: > > Patches 1 and 2 are missing for me, so I couldn't review properly, > > but this diff looks odd. > > It allows sched_cls prog type to attach to tun. > > That means everything that sched_cls progs can do will be done from tun hook? > > We do not have an intention to modify the packet in this steering eBPF. > There is just one function that unavailable for BPF_PROG_TYPE_SOCKET_FILTER > that the eBPF needs to make possible to deliver the hash to the guest > VM - it is 'bpf_set_hash' > > Does it mean that we need to define a new eBPF type for socket filter > operations + set_hash? > > Our problem is that the eBPF calculates 32-bit hash, 16-bit queue > index and 8-bit of hash type. > But it is able to return only 32-bit integer, so in this set of > patches the eBPF returns > queue index and hash type and saves the hash in skb->hash using bpf_set_hash(). > > If this is unacceptable, can you please recommend a better solution? > > > sched_cls assumes l2 and can modify the packet. > > The steering eBPF in TUN module also assumes l2. > > > I think crashes are inevitable. > > > > > -- > > > 2.17.1 > > >
On Tue, Jan 12, 2021 at 12:55 PM Yuri Benditovich <yuri.benditovich@daynix.com> wrote: > > On Tue, Jan 12, 2021 at 10:40 PM Yuri Benditovich > <yuri.benditovich@daynix.com> wrote: > > > > On Tue, Jan 12, 2021 at 9:42 PM Yuri Benditovich > > <yuri.benditovich@daynix.com> wrote: > > > > > > This program type can set skb hash value. It will be useful > > > when the tun will support hash reporting feature if virtio-net. > > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > > --- > > > drivers/net/tun.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > index 7959b5c2d11f..455f7afc1f36 100644 > > > --- a/drivers/net/tun.c > > > +++ b/drivers/net/tun.c > > > @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > > > prog = NULL; > > > } else { > > > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > > > + if (IS_ERR(prog)) > > > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); > > > if (IS_ERR(prog)) > > > return PTR_ERR(prog); > > > } > > > > Comment from Alexei Starovoitov: > > Patches 1 and 2 are missing for me, so I couldn't review properly, > > but this diff looks odd. > > It allows sched_cls prog type to attach to tun. > > That means everything that sched_cls progs can do will be done from tun hook? > > We do not have an intention to modify the packet in this steering eBPF. The intent is irrelevant. Using SCHED_CLS here will let users modify the packet and some users will do so. Hence the tun code has to support it. > There is just one function that unavailable for BPF_PROG_TYPE_SOCKET_FILTER > that the eBPF needs to make possible to deliver the hash to the guest > VM - it is 'bpf_set_hash' > > Does it mean that we need to define a new eBPF type for socket filter > operations + set_hash? > > Our problem is that the eBPF calculates 32-bit hash, 16-bit queue > index and 8-bit of hash type. > But it is able to return only 32-bit integer, so in this set of > patches the eBPF returns > queue index and hash type and saves the hash in skb->hash using bpf_set_hash(). bpf prog can only return a 32-bit integer. That's true. But the prog can use helpers to set any number of bits and variables. bpf_set_hash_v2() with hash, queue and index arguments could fit this purpose, but if you allow it for SCHED_CLS type, tc side of the code should be ready to deal with that too and this extended helper should be meaningful for both tc and tun. In general if the purpose of the prog is to compute three values they better be grouped together. Returned two of them via ORed 32-bit integer and returning 32-bit via bpf_set_hash is an awkward api.
On Wed, Jan 20, 2021 at 8:45 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jan 12, 2021 at 12:55 PM Yuri Benditovich > <yuri.benditovich@daynix.com> wrote: > > > > On Tue, Jan 12, 2021 at 10:40 PM Yuri Benditovich > > <yuri.benditovich@daynix.com> wrote: > > > > > > On Tue, Jan 12, 2021 at 9:42 PM Yuri Benditovich > > > <yuri.benditovich@daynix.com> wrote: > > > > > > > > This program type can set skb hash value. It will be useful > > > > when the tun will support hash reporting feature if virtio-net. > > > > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > > > --- > > > > drivers/net/tun.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > > > > index 7959b5c2d11f..455f7afc1f36 100644 > > > > --- a/drivers/net/tun.c > > > > +++ b/drivers/net/tun.c > > > > @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, > > > > prog = NULL; > > > > } else { > > > > prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); > > > > + if (IS_ERR(prog)) > > > > + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); > > > > if (IS_ERR(prog)) > > > > return PTR_ERR(prog); > > > > } > > > > > > Comment from Alexei Starovoitov: > > > Patches 1 and 2 are missing for me, so I couldn't review properly, > > > but this diff looks odd. > > > It allows sched_cls prog type to attach to tun. > > > That means everything that sched_cls progs can do will be done from tun hook? > > > > We do not have an intention to modify the packet in this steering eBPF. > > The intent is irrelevant. Using SCHED_CLS here will let users modify the packet > and some users will do so. Hence the tun code has to support it. > > > There is just one function that unavailable for BPF_PROG_TYPE_SOCKET_FILTER > > that the eBPF needs to make possible to deliver the hash to the guest > > VM - it is 'bpf_set_hash' > > > > Does it mean that we need to define a new eBPF type for socket filter > > operations + set_hash? > > > > Our problem is that the eBPF calculates 32-bit hash, 16-bit queue > > index and 8-bit of hash type. > > But it is able to return only 32-bit integer, so in this set of > > patches the eBPF returns > > queue index and hash type and saves the hash in skb->hash using bpf_set_hash(). > > bpf prog can only return a 32-bit integer. That's true. > But the prog can use helpers to set any number of bits and variables. > bpf_set_hash_v2() with hash, queue and index arguments could fit this purpose, > but if you allow it for SCHED_CLS type, Do I understand correctly that this means: 1. Creation of new helper like https://lists.linuxfoundation.org/pipermail/bridge/2020-July/013036.html 2. Validation on tun side that the BPF uses only limited subset of helpers available for SCHED_CLS > tc side of the code should be ready to deal with that too and this extended > helper should be meaningful for both tc and tun. > > In general if the purpose of the prog is to compute three values they better be > grouped together. Returned two of them via ORed 32-bit integer and > returning 32-bit via bpf_set_hash is an awkward api.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 7959b5c2d11f..455f7afc1f36 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -2981,6 +2981,8 @@ static int tun_set_ebpf(struct tun_struct *tun, struct tun_prog __rcu **prog_p, prog = NULL; } else { prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SOCKET_FILTER); + if (IS_ERR(prog)) + prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_SCHED_CLS); if (IS_ERR(prog)) return PTR_ERR(prog); }
This program type can set skb hash value. It will be useful when the tun will support hash reporting feature if virtio-net. Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> --- drivers/net/tun.c | 2 ++ 1 file changed, 2 insertions(+)