| Message ID | 1551846958-961-1-git-send-email-wenxu@ucloud.cn |
|---|---|
| State | Rejected |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev] dif-netlink: avoid offload the unsupported USERSPACE action | expand |
Hi, Are there any idear for this patch？ BR wenxu 在 2019-03-06 12:35:58，wenxu@ucloud.cn 写道： > From: wenxu <wenxu@ucloud.cn> > > The hw do/t support USERSPACE action, it will lead many err > logs as following > "ERR|failed to offload flow: Operation not supported" > > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- > lib/dpif-netlink.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 00538e5..46fe022 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -2036,6 +2036,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) > csum_on = tnl_cfg->csum; > } > netdev_close(outdev); > + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) { > + err = EOPNOTSUPP; > + goto out; > } > } > > -- > 1.8.3.1 >
I guess that Simon should look at it. On Mon, Mar 11, 2019 at 09:26:03AM +0800, wenxu wrote: > > Hi, > Are there any idear for this patch？ > > BR > wenxu > > > 在 2019-03-06 12:35:58，wenxu@ucloud.cn 写道： > > > From: wenxu <wenxu@ucloud.cn> > > > > The hw do/t support USERSPACE action, it will lead many err > > logs as following > > "ERR|failed to offload flow: Operation not supported" > > > > Signed-off-by: wenxu <wenxu@ucloud.cn> > > --- > > lib/dpif-netlink.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > > index 00538e5..46fe022 100644 > > --- a/lib/dpif-netlink.c > > +++ b/lib/dpif-netlink.c > > @@ -2036,6 +2036,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) > > csum_on = tnl_cfg->csum; > > } > > netdev_close(outdev); > > + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) { > > + err = EOPNOTSUPP; > > + goto out; > > } > > } > > > > -- > > 1.8.3.1 > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Wenxu,
I think that the error you are seeing is the expected behaviour whereby
netdev_tc_flow_put() returns EOPNOTSUPP when it receives a flow
whose offload is not supported. And it seems to me that is the
correct place for this to be handled.
I am referring to the following code around line 1555 of
lib/netdev-tc-offloads.c
} else {
VLOG_DBG_RL(&rl, "unsupported put action type: %d",
nl_attr_type(nla));
return EOPNOTSUPP;
}
I do, however, wonder if the log message you report should be of level info
rather than error in the case where the error is EOPNOTSUPP: this isn't an
error in the sense that the system is malfunctioning but rather lower
layers are reporting that a flow can't be offloaded because it is not
supported which is a part of the design of support for offloads.
Kind regards,
Simon
On Mon, Mar 18, 2019 at 01:45:20PM -0700, Ben Pfaff wrote:
> I guess that Simon should look at it.
>
> On Mon, Mar 11, 2019 at 09:26:03AM +0800, wenxu wrote:
> >
> > Hi,
> > Are there any idear for this patch？
> >
> > BR
> > wenxu
> >
> >
> > 在 2019-03-06 12:35:58，wenxu@ucloud.cn 写道：
> >
> > > From: wenxu <wenxu@ucloud.cn>
> > >
> > > The hw do/t support USERSPACE action, it will lead many err
> > > logs as following
> > > "ERR|failed to offload flow: Operation not supported"
> > >
> > > Signed-off-by: wenxu <wenxu@ucloud.cn>
> > > ---
> > > lib/dpif-netlink.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > > index 00538e5..46fe022 100644
> > > --- a/lib/dpif-netlink.c
> > > +++ b/lib/dpif-netlink.c
> > > @@ -2036,6 +2036,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
> > > csum_on = tnl_cfg->csum;
> > > }
> > > netdev_close(outdev);
> > > + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
> > > + err = EOPNOTSUPP;
> > > + goto out;
> > > }
> > > }
> > >
> > > --
> > > 1.8.3.1
> > >
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 3/19/2019 4:21 PM, Simon Horman wrote: > Hi Wenxu, > > I think that the error you are seeing is the expected behaviour whereby > netdev_tc_flow_put() returns EOPNOTSUPP when it receives a flow > whose offload is not supported. And it seems to me that is the > correct place for this to be handled. > > I am referring to the following code around line 1555 of > lib/netdev-tc-offloads.c > > } else { > VLOG_DBG_RL(&rl, "unsupported put action type: %d", > nl_attr_type(nla)); > return EOPNOTSUPP; > } > > I do, however, wonder if the log message you report should be of level info > rather than error in the case where the error is EOPNOTSUPP: this isn't an > error in the sense that the system is malfunctioning but rather lower > layers are reporting that a flow can't be offloaded because it is not > supported which is a part of the design of support for offloads. It maybe better for depreciate the log message for some EOPNOTSUPP case (USERSPACE action)? It is not an err case the the lower layer is not supported. It indeed can't support by hw. The user will not care about this and don't expect receive the logs. On the online system, it will lead many unused log for user. > > Kind regards, > Simon > > > On Mon, Mar 18, 2019 at 01:45:20PM -0700, Ben Pfaff wrote: >> I guess that Simon should look at it. >> >> On Mon, Mar 11, 2019 at 09:26:03AM +0800, wenxu wrote: >>> Hi, >>> Are there any idear for this patch？ >>> >>> BR >>> wenxu >>> >>> >>> 在 2019-03-06 12:35:58，wenxu@ucloud.cn 写道： >>> >>>> From: wenxu <wenxu@ucloud.cn> >>>> >>>> The hw do/t support USERSPACE action, it will lead many err >>>> logs as following >>>> "ERR|failed to offload flow: Operation not supported" >>>> >>>> Signed-off-by: wenxu <wenxu@ucloud.cn> >>>> --- >>>> lib/dpif-netlink.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>>> index 00538e5..46fe022 100644 >>>> --- a/lib/dpif-netlink.c >>>> +++ b/lib/dpif-netlink.c >>>> @@ -2036,6 +2036,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) >>>> csum_on = tnl_cfg->csum; >>>> } >>>> netdev_close(outdev); >>>> + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) { >>>> + err = EOPNOTSUPP; >>>> + goto out; >>>> } >>>> } >>>> >>>> -- >>>> 1.8.3.1 >>>> >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Mar 19, 2019 at 05:56:12PM +0800, wenxu wrote: > > On 3/19/2019 4:21 PM, Simon Horman wrote: > > Hi Wenxu, > > > > I think that the error you are seeing is the expected behaviour whereby > > netdev_tc_flow_put() returns EOPNOTSUPP when it receives a flow > > whose offload is not supported. And it seems to me that is the > > correct place for this to be handled. > > > > I am referring to the following code around line 1555 of > > lib/netdev-tc-offloads.c > > > > } else { > > VLOG_DBG_RL(&rl, "unsupported put action type: %d", > > nl_attr_type(nla)); > > return EOPNOTSUPP; > > } > > > > I do, however, wonder if the log message you report should be of level info > > rather than error in the case where the error is EOPNOTSUPP: this isn't an > > error in the sense that the system is malfunctioning but rather lower > > layers are reporting that a flow can't be offloaded because it is not > > supported which is a part of the design of support for offloads. > > It maybe better for depreciate the log message for some EOPNOTSUPP case (USERSPACE action)? > > It is not an err case the the lower layer is not supported. It indeed can't support by hw. The user will not > > care about this and don't expect receive the logs. On the online system, it will lead many unused log for user. Hi Wenxu, I think there is some value in logging the message at a lower priority, say debug rather than err, and to allow users to see the run-time behaviour if their log filter is setup accordingly. Looking at the code it seems that ENOSPC may also a good candidate for this kind of treatment. Something like the following (compile tested only). diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 00538e5e47d9..92dd003452a9 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2067,6 +2067,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) VLOG_DBG("added flow"); } else if (err != EEXIST) { + enum vlog_level level; struct netdev *oor_netdev = NULL; if (err == ENOSPC && netdev_is_offload_rebalance_policy_enabled()) { /* @@ -2082,8 +2083,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) } netdev_set_hw_info(oor_netdev, HW_INFO_TYPE_OOR, true); } - VLOG_ERR_RL(&rl, "failed to offload flow: %s: %s", ovs_strerror(err), - (oor_netdev ? oor_netdev->name : dev->name)); + level = (err == ENOSPC || err == EOPNOTSUPP) ? VLL_DBG : VLL_ERR; + VLOG_RL(&rl, level, "failed to offload flow: %s: %s", + ovs_strerror(err), + (oor_netdev ? oor_netdev->name : dev->name)); } out:
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 00538e5..46fe022 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2036,6 +2036,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) csum_on = tnl_cfg->csum; } netdev_close(outdev); + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) { + err = EOPNOTSUPP; + goto out; } }