| Message ID | 164362528978.2824752.6964902999127156.stgit@ebuild |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | netdev-offload-tc: Fix various tc-offload related problems | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2022-01-31 12:34 PM, Eelco Chaudron wrote: > Verify that the returned ifindex by netdev_get_ifindex() is valid. > This might not be the case in the ERSPAN port scenario, which can > not be offloaded. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/netdev-offload-tc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 9845e8d3f..8135441c6 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, > return ENODEV; > } > action->out.ifindex_out = netdev_get_ifindex(outdev); > + > + if (action->out.ifindex_out < 0) { > + VLOG_DBG_RL(&rl, > + "Can't find ifindex for output port %s, error %d", > + netdev_get_name(outdev), action->out.ifindex_out); > + > + return -action->out.ifindex_out; > + } > + > action->out.ingress = is_internal_port(netdev_get_type(outdev)); > action->type = TC_ACT_OUTPUT; > flower.action_count++; > Acked-by: Roi Dayan <roid@nvidia.com>
On 1/31/22 11:34, Eelco Chaudron wrote: > Verify that the returned ifindex by netdev_get_ifindex() is valid. > This might not be the case in the ERSPAN port scenario, which can > not be offloaded. Why exactly we can't get ifindex of the kernel interface? > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/netdev-offload-tc.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 9845e8d3f..8135441c6 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, > return ENODEV; > } > action->out.ifindex_out = netdev_get_ifindex(outdev); > + > + if (action->out.ifindex_out < 0) { > + VLOG_DBG_RL(&rl, > + "Can't find ifindex for output port %s, error %d", > + netdev_get_name(outdev), action->out.ifindex_out); > + We're leaking netdev reference here. Shouldn't we check netdev_flow_api_equals(netdev, outdev) here instead? This way we will be sure that flow API was successfully initialized for the outdev and we're not mixing different offload providers. > + return -action->out.ifindex_out; > + } > + > action->out.ingress = is_internal_port(netdev_get_type(outdev)); > action->type = TC_ACT_OUTPUT; > flower.action_count++; >
On 2/17/22 13:57, Ilya Maximets wrote: > On 1/31/22 11:34, Eelco Chaudron wrote: >> Verify that the returned ifindex by netdev_get_ifindex() is valid. >> This might not be the case in the ERSPAN port scenario, which can >> not be offloaded. > > Why exactly we can't get ifindex of the kernel interface? Hmm. I see that netdev_vport_tunnel_register() doesn't register the .get_ifindex callback for erspan and some other tunnel types, but I'm not sure why... > >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/netdev-offload-tc.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 9845e8d3f..8135441c6 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, >> return ENODEV; >> } >> action->out.ifindex_out = netdev_get_ifindex(outdev); >> + >> + if (action->out.ifindex_out < 0) { >> + VLOG_DBG_RL(&rl, >> + "Can't find ifindex for output port %s, error %d", >> + netdev_get_name(outdev), action->out.ifindex_out); >> + > > We're leaking netdev reference here. > > Shouldn't we check netdev_flow_api_equals(netdev, outdev) here instead? > This way we will be sure that flow API was successfully initialized for > the outdev and we're not mixing different offload providers. > >> + return -action->out.ifindex_out; >> + } >> + >> action->out.ingress = is_internal_port(netdev_get_type(outdev)); >> action->type = TC_ACT_OUTPUT; >> flower.action_count++; >>
On 17 Feb 2022, at 16:07, Ilya Maximets wrote: > On 2/17/22 13:57, Ilya Maximets wrote: >> On 1/31/22 11:34, Eelco Chaudron wrote: >>> Verify that the returned ifindex by netdev_get_ifindex() is valid. >>> This might not be the case in the ERSPAN port scenario, which can >>> not be offloaded. >> >> Why exactly we can't get ifindex of the kernel interface? > > Hmm. I see that netdev_vport_tunnel_register() doesn't register > the .get_ifindex callback for erspan and some other tunnel types, > but I'm not sure why... Same here. I could not think of a reason. But did not want to break anything by adding it. >>> >>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >>> lib/netdev-offload-tc.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>> index 9845e8d3f..8135441c6 100644 >>> --- a/lib/netdev-offload-tc.c >>> +++ b/lib/netdev-offload-tc.c >>> @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, >>> return ENODEV; >>> } >>> action->out.ifindex_out = netdev_get_ifindex(outdev); >>> + >>> + if (action->out.ifindex_out < 0) { >>> + VLOG_DBG_RL(&rl, >>> + "Can't find ifindex for output port %s, error %d", >>> + netdev_get_name(outdev), action->out.ifindex_out); >>> + >> >> We're leaking netdev reference here. Good catch! >> Shouldn't we check netdev_flow_api_equals(netdev, outdev) here instead? >> This way we will be sure that flow API was successfully initialized for >> the outdev and we're not mixing different offload providers. Decided to add it, and keep the index check also. So it will look something like: @@ -1841,7 +1841,24 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, VLOG_DBG_RL(&rl, "Can't find netdev for output port %d", port); return ENODEV; } + + if (!netdev_flow_api_equals(netdev, outdev)) { + VLOG_DBG_RL(&rl, + "Egress port %s is using a different flow API " + "provider", netdev_get_name(outdev)); + netdev_close(outdev); + return EOPNOTSUPP; + } + action->out.ifindex_out = netdev_get_ifindex(outdev); + if (action->out.ifindex_out < 0) { + VLOG_DBG_RL(&rl, + "Can't find ifindex for output port %s, error %d", + netdev_get_name(outdev), action->out.ifindex_out); + netdev_close(outdev); + return -action->out.ifindex_out; + } + >>> + return -action->out.ifindex_out; >>> + } >>> + >>> action->out.ingress = is_internal_port(netdev_get_type(outdev)); >>> action->type = TC_ACT_OUTPUT; >>> flower.action_count++; >>>
On 2/21/22 11:53, Eelco Chaudron wrote: > > > On 17 Feb 2022, at 16:07, Ilya Maximets wrote: > >> On 2/17/22 13:57, Ilya Maximets wrote: >>> On 1/31/22 11:34, Eelco Chaudron wrote: >>>> Verify that the returned ifindex by netdev_get_ifindex() is valid. >>>> This might not be the case in the ERSPAN port scenario, which can >>>> not be offloaded. >>> >>> Why exactly we can't get ifindex of the kernel interface? >> >> Hmm. I see that netdev_vport_tunnel_register() doesn't register >> the .get_ifindex callback for erspan and some other tunnel types, >> but I'm not sure why... > > Same here. I could not think of a reason. But did not want to break anything by adding it. > >>>> >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>>> --- >>>> lib/netdev-offload-tc.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>> index 9845e8d3f..8135441c6 100644 >>>> --- a/lib/netdev-offload-tc.c >>>> +++ b/lib/netdev-offload-tc.c >>>> @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, >>>> return ENODEV; >>>> } >>>> action->out.ifindex_out = netdev_get_ifindex(outdev); >>>> + >>>> + if (action->out.ifindex_out < 0) { >>>> + VLOG_DBG_RL(&rl, >>>> + "Can't find ifindex for output port %s, error %d", >>>> + netdev_get_name(outdev), action->out.ifindex_out); >>>> + >>> >>> We're leaking netdev reference here. > > Good catch! > >>> Shouldn't we check netdev_flow_api_equals(netdev, outdev) here instead? >>> This way we will be sure that flow API was successfully initialized for >>> the outdev and we're not mixing different offload providers. > > Decided to add it, and keep the index check also. So it will look something like: > > @@ -1841,7 +1841,24 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, > VLOG_DBG_RL(&rl, "Can't find netdev for output port %d", port); > return ENODEV; > } > + > + if (!netdev_flow_api_equals(netdev, outdev)) { > + VLOG_DBG_RL(&rl, > + "Egress port %s is using a different flow API " > + "provider", netdev_get_name(outdev)); I'm not sure if the message is fully correct as egress port may not have any offload provider initialized. Maybe something like: "Flow API provider mismatch between ingress (%s) and egress (%s) ports" or "Egress port %s doesn't have or is using a different ..." (<-- this one should be phrased differently, I guess, but it's just an example). What do you think? > + netdev_close(outdev); > + return EOPNOTSUPP; > + } > + > action->out.ifindex_out = netdev_get_ifindex(outdev); > + if (action->out.ifindex_out < 0) { > + VLOG_DBG_RL(&rl, > + "Can't find ifindex for output port %s, error %d", > + netdev_get_name(outdev), action->out.ifindex_out); > + netdev_close(outdev); > + return -action->out.ifindex_out; > + } > + OK. Should work. I don't think that ifindex check is necessary here as initialization of tc offload provider requires having a valid ifindex, but I agree that extra check should not make any harm. > >>>> + return -action->out.ifindex_out; >>>> + } >>>> + >>>> action->out.ingress = is_internal_port(netdev_get_type(outdev)); >>>> action->type = TC_ACT_OUTPUT; >>>> flower.action_count++; >>>> >
On 21 Feb 2022, at 12:45, Ilya Maximets wrote: > On 2/21/22 11:53, Eelco Chaudron wrote: >> >> >> On 17 Feb 2022, at 16:07, Ilya Maximets wrote: >> >>> On 2/17/22 13:57, Ilya Maximets wrote: >>>> On 1/31/22 11:34, Eelco Chaudron wrote: >>>>> Verify that the returned ifindex by netdev_get_ifindex() is valid. >>>>> This might not be the case in the ERSPAN port scenario, which can >>>>> not be offloaded. >>>> >>>> Why exactly we can't get ifindex of the kernel interface? >>> >>> Hmm. I see that netdev_vport_tunnel_register() doesn't register >>> the .get_ifindex callback for erspan and some other tunnel types, >>> but I'm not sure why... >> >> Same here. I could not think of a reason. But did not want to break anything by adding it. >> >>>>> >>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>>>> --- >>>>> lib/netdev-offload-tc.c | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>>> >>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>>> index 9845e8d3f..8135441c6 100644 >>>>> --- a/lib/netdev-offload-tc.c >>>>> +++ b/lib/netdev-offload-tc.c >>>>> @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, >>>>> return ENODEV; >>>>> } >>>>> action->out.ifindex_out = netdev_get_ifindex(outdev); >>>>> + >>>>> + if (action->out.ifindex_out < 0) { >>>>> + VLOG_DBG_RL(&rl, >>>>> + "Can't find ifindex for output port %s, error %d", >>>>> + netdev_get_name(outdev), action->out.ifindex_out); >>>>> + >>>> >>>> We're leaking netdev reference here. >> >> Good catch! >> >>>> Shouldn't we check netdev_flow_api_equals(netdev, outdev) here instead? >>>> This way we will be sure that flow API was successfully initialized for >>>> the outdev and we're not mixing different offload providers. >> >> Decided to add it, and keep the index check also. So it will look something like: >> >> @@ -1841,7 +1841,24 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, >> VLOG_DBG_RL(&rl, "Can't find netdev for output port %d", port); >> return ENODEV; >> } >> + >> + if (!netdev_flow_api_equals(netdev, outdev)) { >> + VLOG_DBG_RL(&rl, >> + "Egress port %s is using a different flow API " >> + "provider", netdev_get_name(outdev)); > > I'm not sure if the message is fully correct as egress port may not > have any offload provider initialized. Maybe something like: > "Flow API provider mismatch between ingress (%s) and egress (%s) ports" > or > "Egress port %s doesn't have or is using a different ..." (<-- this one > should be phrased differently, I guess, but it's just an example). > > What do you think? Took the first suggestion. I will send it as part of v3... >> + netdev_close(outdev); >> + return EOPNOTSUPP; >> + } >> + >> action->out.ifindex_out = netdev_get_ifindex(outdev); >> + if (action->out.ifindex_out < 0) { >> + VLOG_DBG_RL(&rl, >> + "Can't find ifindex for output port %s, error %d", >> + netdev_get_name(outdev), action->out.ifindex_out); >> + netdev_close(outdev); >> + return -action->out.ifindex_out; >> + } >> + > > OK. Should work. > I don't think that ifindex check is necessary here as initialization > of tc offload provider requires having a valid ifindex, but I agree > that extra check should not make any harm. > >> >>>>> + return -action->out.ifindex_out; >>>>> + } >>>>> + >>>>> action->out.ingress = is_internal_port(netdev_get_type(outdev)); >>>>> action->type = TC_ACT_OUTPUT; >>>>> flower.action_count++; >>>>> >>
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 9845e8d3f..8135441c6 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -1842,6 +1842,15 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, return ENODEV; } action->out.ifindex_out = netdev_get_ifindex(outdev); + + if (action->out.ifindex_out < 0) { + VLOG_DBG_RL(&rl, + "Can't find ifindex for output port %s, error %d", + netdev_get_name(outdev), action->out.ifindex_out); + + return -action->out.ifindex_out; + } + action->out.ingress = is_internal_port(netdev_get_type(outdev)); action->type = TC_ACT_OUTPUT; flower.action_count++;
Verify that the returned ifindex by netdev_get_ifindex() is valid. This might not be the case in the ERSPAN port scenario, which can not be offloaded. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/netdev-offload-tc.c | 9 +++++++++ 1 file changed, 9 insertions(+)