| Message ID | 20190703045806.5357-1-elibr@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,V2,1/1] dpif-netlink: Log eth type 0x1234 not offloadable | expand |
On Wed, Jul 03, 2019 at 04:58:06AM +0000, Eli Britstein wrote: > Ethernet type 0x1234 is used for testing and not being offloadable. For > testing offloadable features, log about using this value. > > Signed-off-by: Eli Britstein <elibr@mellanox.com> > Acked-by: Roi Dayan <roid@mellanox.com> > Signed-off-by: Eli Britstein <elibr@mellanox.com> Acked-by: Ben Pfaff <blp@ovn.org>
On 2019-07-03 9:11 PM, Ben Pfaff wrote: > On Wed, Jul 03, 2019 at 04:58:06AM +0000, Eli Britstein wrote: >> Ethernet type 0x1234 is used for testing and not being offloadable. For >> testing offloadable features, log about using this value. >> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> Acked-by: Roi Dayan <roid@mellanox.com> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> > > Acked-by: Ben Pfaff <blp@ovn.org> > ping. can we merge this? thanks
ping On 7/11/2019 4:11 PM, Roi Dayan wrote: > > On 2019-07-03 9:11 PM, Ben Pfaff wrote: >> On Wed, Jul 03, 2019 at 04:58:06AM +0000, Eli Britstein wrote: >>> Ethernet type 0x1234 is used for testing and not being offloadable. For >>> testing offloadable features, log about using this value. >>> >>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>> Acked-by: Roi Dayan <roid@mellanox.com> >>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> Acked-by: Ben Pfaff <blp@ovn.org> >> > ping. can we merge this? > thanks
On 03.07.2019 7:58, Eli Britstein wrote: > Ethernet type 0x1234 is used for testing and not being offloadable. For > testing offloadable features, log about using this value. > > Signed-off-by: Eli Britstein <elibr@mellanox.com> > Acked-by: Roi Dayan <roid@mellanox.com> > Signed-off-by: Eli Britstein <elibr@mellanox.com> > Acked-by: Ben Pfaff <blp@ovn.org> > --- > lib/dpif-netlink.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index ba80a0079..a0d51ae61 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) > > /* When we try to install a dummy flow from a probed feature. */ > if (match.flow.dl_type == htons(0x1234)) { > + VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable"); > return EOPNOTSUPP; > } But what is the purpose of this patch? What is the use case? Actually, it looks like we need to just remove above 'if' statement entirely. Just a few lines above there is a check if we are probing or installing real flow: if (put->flags & DPIF_FP_PROBE) { return EOPNOTSUPP; } So, we will never get there while probing. But why we're restricting this flow type from being offloaded? 'netdev_flow_put' will refuse to offload if it doesn't know that flow type, but this shouldn't be done here. In case we have a dummy flow without DPIF_FP_PROBE flag set, we need to fix upper layers. Is it the case? Best regards, Ilya Maximets.
On 7/30/2019 1:10 PM, Ilya Maximets wrote: > On 03.07.2019 7:58, Eli Britstein wrote: >> Ethernet type 0x1234 is used for testing and not being offloadable. For >> testing offloadable features, log about using this value. >> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> Acked-by: Roi Dayan <roid@mellanox.com> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> Acked-by: Ben Pfaff <blp@ovn.org> >> --- >> lib/dpif-netlink.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index ba80a0079..a0d51ae61 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) >> >> /* When we try to install a dummy flow from a probed feature. */ >> if (match.flow.dl_type == htons(0x1234)) { >> + VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable"); >> return EOPNOTSUPP; >> } > But what is the purpose of this patch? What is the use case? RH wanted to test that dl_type is offloaded. Coincidentally, they used 0x1234, and it was not offloaded, and they didn't understand why, and suggested a log message. > > Actually, it looks like we need to just remove above 'if' statement > entirely. Just a few lines above there is a check if we are probing > or installing real flow: > > if (put->flags & DPIF_FP_PROBE) { > return EOPNOTSUPP; > } > > So, we will never get there while probing. But why we're restricting > this flow type from being offloaded? 'netdev_flow_put' will refuse > to offload if it doesn't know that flow type, but this shouldn't be > done here. > > In case we have a dummy flow without DPIF_FP_PROBE flag set, we need > to fix upper layers. Is it the case? I didn't look into it why we restrict it and if there is a real reason why in this layer. You may be right, but I don't know. > > Best regards, Ilya Maximets.
On 30.07.2019 13:23, Eli Britstein wrote: > > On 7/30/2019 1:10 PM, Ilya Maximets wrote: >> On 03.07.2019 7:58, Eli Britstein wrote: >>> Ethernet type 0x1234 is used for testing and not being offloadable. For >>> testing offloadable features, log about using this value. >>> >>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>> Acked-by: Roi Dayan <roid@mellanox.com> >>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>> Acked-by: Ben Pfaff <blp@ovn.org> >>> --- >>> lib/dpif-netlink.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>> index ba80a0079..a0d51ae61 100644 >>> --- a/lib/dpif-netlink.c >>> +++ b/lib/dpif-netlink.c >>> @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) >>> >>> /* When we try to install a dummy flow from a probed feature. */ >>> if (match.flow.dl_type == htons(0x1234)) { >>> + VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable"); >>> return EOPNOTSUPP; >>> } >> But what is the purpose of this patch? What is the use case? > RH wanted to test that dl_type is offloaded. Coincidentally, they used > 0x1234, and it was not offloaded, and they didn't understand why, and > suggested a log message. I'll take a closer look, but it seems that we just need to remove this 'if' statement and allow oflloading. >> >> Actually, it looks like we need to just remove above 'if' statement >> entirely. Just a few lines above there is a check if we are probing >> or installing real flow: >> >> if (put->flags & DPIF_FP_PROBE) { >> return EOPNOTSUPP; >> } >> >> So, we will never get there while probing. But why we're restricting >> this flow type from being offloaded? 'netdev_flow_put' will refuse >> to offload if it doesn't know that flow type, but this shouldn't be >> done here. >> >> In case we have a dummy flow without DPIF_FP_PROBE flag set, we need >> to fix upper layers. Is it the case? > > I didn't look into it why we restrict it and if there is a real reason > why in this layer. You may be right, but I don't know. > >> >> Best regards, Ilya Maximets.
On 30.07.2019 13:30, Ilya Maximets wrote: > On 30.07.2019 13:23, Eli Britstein wrote: >> >> On 7/30/2019 1:10 PM, Ilya Maximets wrote: >>> On 03.07.2019 7:58, Eli Britstein wrote: >>>> Ethernet type 0x1234 is used for testing and not being offloadable. For >>>> testing offloadable features, log about using this value. >>>> >>>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>>> Acked-by: Ben Pfaff <blp@ovn.org> >>>> --- >>>> lib/dpif-netlink.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>>> index ba80a0079..a0d51ae61 100644 >>>> --- a/lib/dpif-netlink.c >>>> +++ b/lib/dpif-netlink.c >>>> @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) >>>> >>>> /* When we try to install a dummy flow from a probed feature. */ >>>> if (match.flow.dl_type == htons(0x1234)) { >>>> + VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable"); >>>> return EOPNOTSUPP; >>>> } >>> But what is the purpose of this patch? What is the use case? >> RH wanted to test that dl_type is offloaded. Coincidentally, they used >> 0x1234, and it was not offloaded, and they didn't understand why, and >> suggested a log message. > > I'll take a closer look, but it seems that we just need to remove this > 'if' statement and allow oflloading. 'dpif_probe_feature' always has DPIF_FP_PROBE flag set. Other probing code uses dpif_execute() which uses DPIF_OP_EXECUTE, hence never calls parse_flow_put(). So, this 'if' statement is wrong and should be deleted as it only forbids offloading of the real legitimate flows with dl_type 0x1234. Dummy flows never reaches this code. > >>> >>> Actually, it looks like we need to just remove above 'if' statement >>> entirely. Just a few lines above there is a check if we are probing >>> or installing real flow: >>> >>> if (put->flags & DPIF_FP_PROBE) { >>> return EOPNOTSUPP; >>> } >>> >>> So, we will never get there while probing. But why we're restricting >>> this flow type from being offloaded? 'netdev_flow_put' will refuse >>> to offload if it doesn't know that flow type, but this shouldn't be >>> done here. >>> >>> In case we have a dummy flow without DPIF_FP_PROBE flag set, we need >>> to fix upper layers. Is it the case? >> >> I didn't look into it why we restrict it and if there is a real reason >> why in this layer. You may be right, but I don't know. >> >>> >>> Best regards, Ilya Maximets.
On 7/30/2019 3:19 PM, Ilya Maximets wrote: > On 30.07.2019 13:30, Ilya Maximets wrote: >> On 30.07.2019 13:23, Eli Britstein wrote: >>> On 7/30/2019 1:10 PM, Ilya Maximets wrote: >>>> On 03.07.2019 7:58, Eli Britstein wrote: >>>>> Ethernet type 0x1234 is used for testing and not being offloadable. For >>>>> testing offloadable features, log about using this value. >>>>> >>>>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>>>> Acked-by: Ben Pfaff <blp@ovn.org> >>>>> --- >>>>> lib/dpif-netlink.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>>>> index ba80a0079..a0d51ae61 100644 >>>>> --- a/lib/dpif-netlink.c >>>>> +++ b/lib/dpif-netlink.c >>>>> @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) >>>>> >>>>> /* When we try to install a dummy flow from a probed feature. */ >>>>> if (match.flow.dl_type == htons(0x1234)) { >>>>> + VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable"); >>>>> return EOPNOTSUPP; >>>>> } >>>> But what is the purpose of this patch? What is the use case? >>> RH wanted to test that dl_type is offloaded. Coincidentally, they used >>> 0x1234, and it was not offloaded, and they didn't understand why, and >>> suggested a log message. >> I'll take a closer look, but it seems that we just need to remove this >> 'if' statement and allow oflloading. > 'dpif_probe_feature' always has DPIF_FP_PROBE flag set. Other probing code uses > dpif_execute() which uses DPIF_OP_EXECUTE, hence never calls parse_flow_put(). > So, this 'if' statement is wrong and should be deleted as it only forbids > offloading of the real legitimate flows with dl_type 0x1234. Dummy flows never > reaches this code. Couldn't find any reason for it, I even looked at diff from my commit that introduced it. Seems safe to remove it. >>>> Actually, it looks like we need to just remove above 'if' statement >>>> entirely. Just a few lines above there is a check if we are probing >>>> or installing real flow: >>>> >>>> if (put->flags & DPIF_FP_PROBE) { >>>> return EOPNOTSUPP; >>>> } >>>> >>>> So, we will never get there while probing. But why we're restricting >>>> this flow type from being offloaded? 'netdev_flow_put' will refuse >>>> to offload if it doesn't know that flow type, but this shouldn't be >>>> done here. >>>> >>>> In case we have a dummy flow without DPIF_FP_PROBE flag set, we need >>>> to fix upper layers. Is it the case? >>> I didn't look into it why we restrict it and if there is a real reason >>> why in this layer. You may be right, but I don't know. >>> >>>> Best regards, Ilya Maximets.
On 30.07.2019 16:50, Paul Blakey wrote: > > On 7/30/2019 3:19 PM, Ilya Maximets wrote: >> On 30.07.2019 13:30, Ilya Maximets wrote: >>> On 30.07.2019 13:23, Eli Britstein wrote: >>>> On 7/30/2019 1:10 PM, Ilya Maximets wrote: >>>>> On 03.07.2019 7:58, Eli Britstein wrote: >>>>>> Ethernet type 0x1234 is used for testing and not being offloadable. For >>>>>> testing offloadable features, log about using this value. >>>>>> >>>>>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>>>>> Acked-by: Roi Dayan <roid@mellanox.com> >>>>>> Signed-off-by: Eli Britstein <elibr@mellanox.com> >>>>>> Acked-by: Ben Pfaff <blp@ovn.org> >>>>>> --- >>>>>> lib/dpif-netlink.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>>>>> index ba80a0079..a0d51ae61 100644 >>>>>> --- a/lib/dpif-netlink.c >>>>>> +++ b/lib/dpif-netlink.c >>>>>> @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) >>>>>> >>>>>> /* When we try to install a dummy flow from a probed feature. */ >>>>>> if (match.flow.dl_type == htons(0x1234)) { >>>>>> + VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable"); >>>>>> return EOPNOTSUPP; >>>>>> } >>>>> But what is the purpose of this patch? What is the use case? >>>> RH wanted to test that dl_type is offloaded. Coincidentally, they used >>>> 0x1234, and it was not offloaded, and they didn't understand why, and >>>> suggested a log message. >>> I'll take a closer look, but it seems that we just need to remove this >>> 'if' statement and allow oflloading. >> 'dpif_probe_feature' always has DPIF_FP_PROBE flag set. Other probing code uses >> dpif_execute() which uses DPIF_OP_EXECUTE, hence never calls parse_flow_put(). >> So, this 'if' statement is wrong and should be deleted as it only forbids >> offloading of the real legitimate flows with dl_type 0x1234. Dummy flows never >> reaches this code. > > Couldn't find any reason for it, I even looked at diff from my commit > that introduced it. > > Seems safe to remove it. OK. Thanks, Paul. I'll prepare a patch. > >>>>> Actually, it looks like we need to just remove above 'if' statement >>>>> entirely. Just a few lines above there is a check if we are probing >>>>> or installing real flow: >>>>> >>>>> if (put->flags & DPIF_FP_PROBE) { >>>>> return EOPNOTSUPP; >>>>> } >>>>> >>>>> So, we will never get there while probing. But why we're restricting >>>>> this flow type from being offloaded? 'netdev_flow_put' will refuse >>>>> to offload if it doesn't know that flow type, but this shouldn't be >>>>> done here. >>>>> >>>>> In case we have a dummy flow without DPIF_FP_PROBE flag set, we need >>>>> to fix upper layers. Is it the case? >>>> I didn't look into it why we restrict it and if there is a real reason >>>> why in this layer. You may be right, but I don't know. >>>> >>>>> Best regards, Ilya Maximets.
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index ba80a0079..a0d51ae61 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) /* When we try to install a dummy flow from a probed feature. */ if (match.flow.dl_type == htons(0x1234)) { + VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable"); return EOPNOTSUPP; }