[ovs-dev] dif-netlink: avoid offload the unsupported USERSPACE action
diff mbox series

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
Related show

Commit Message

wenxu March 6, 2019, 4:35 a.m. UTC
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(+)

Comments

wenxu March 11, 2019, 1:26 a.m. UTC | #1
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
>
Ben Pfaff March 18, 2019, 8:45 p.m. UTC | #2
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
Simon Horman March 19, 2019, 8:21 a.m. UTC | #3
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
wenxu March 19, 2019, 9:56 a.m. UTC | #4
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
Simon Horman March 19, 2019, 10:42 a.m. UTC | #5
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:

Patch
diff mbox series

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;
         }
     }