diff mbox series

[ovs-dev,v2] netlink: ignore IFLA_WIRELESS events

Message ID 20210114090938.6996-1-kazikcz@gmail.com
State New
Headers show
Series [ovs-dev,v2] netlink: ignore IFLA_WIRELESS events | expand

Commit Message

Michal Kazior Jan. 14, 2021, 9:09 a.m. UTC
From: Michal Kazior <michal@plume.com>

Some older wireless drivers - ones relying on the
old and long deprecated wireless extension ioctl
system - can generate quite a bit of IFLA_WIRELESS
events depending on their configuration and
runtime conditions. These are delivered as
RTNLGRP_LINK via RTM_NEWLINK messages.

These tend to be relatively easily identifiable
because they report the change mask being 0. This
isn't guaranteed but in practice it shouldn't be a
problem. None of the wireless events that I ever
observed actually carry any unique information
about netdev states that ovs-vswitchd is
interested in. Hence ignoring these shouldn't
cause any problems.

These events can be responsible for a significant
CPU churn as ovs-vswitchd attempts to do plenty of
work for each and every netlink message regardless
of what that message carries. On low-end devices
such as consumer-grade routers these can lead to a
lot of CPU cycles being wasted, adding up to heat
output and reducing performance.

It could be argued that wireless drivers in
question should be fixed, but that isn't exactly a
trivial thing to do. Patching ovs seems far more
viable while still making sense.

This change requires a slight tweak to the netlink
abstraction code so that the events can be
discarded as soon as possible before getting a
chance to cause the churn.

Signed-off-by: Michal Kazior <michal@plume.com>
---

Notes:
    v2:
     - fix bracing style [0day robot / checkpatch]

 lib/netdev-linux.c     |  4 ++--
 lib/netlink-notifier.c |  4 +++-
 lib/rtnetlink.c        | 24 +++++++++++++++++++++---
 lib/rtnetlink.h        |  2 +-
 4 files changed, 27 insertions(+), 7 deletions(-)

Comments

Gregory Rose Feb. 8, 2021, 11:58 p.m. UTC | #1
On 1/14/2021 1:09 AM, Michal Kazior wrote:
> From: Michal Kazior <michal@plume.com>
> 
> Some older wireless drivers - ones relying on the
> old and long deprecated wireless extension ioctl
> system - can generate quite a bit of IFLA_WIRELESS
> events depending on their configuration and
> runtime conditions. These are delivered as
> RTNLGRP_LINK via RTM_NEWLINK messages.
> 
> These tend to be relatively easily identifiable
> because they report the change mask being 0. This
> isn't guaranteed but in practice it shouldn't be a
> problem. None of the wireless events that I ever
> observed actually carry any unique information
> about netdev states that ovs-vswitchd is
> interested in. Hence ignoring these shouldn't
> cause any problems.
> 
> These events can be responsible for a significant
> CPU churn as ovs-vswitchd attempts to do plenty of
> work for each and every netlink message regardless
> of what that message carries. On low-end devices
> such as consumer-grade routers these can lead to a
> lot of CPU cycles being wasted, adding up to heat
> output and reducing performance.
> 
> It could be argued that wireless drivers in
> question should be fixed, but that isn't exactly a
> trivial thing to do. Patching ovs seems far more
> viable while still making sense.
> 
> This change requires a slight tweak to the netlink
> abstraction code so that the events can be
> discarded as soon as possible before getting a
> chance to cause the churn.
> 
> Signed-off-by: Michal Kazior <michal@plume.com>

Hi Michal,

The patch looks fine to me.  Applies cleanly to master
and did not cause any regressions in 'make check'.

I'm curious since I don't ever use OVS with wireless ports
if there is somewhere this issue has been reported.  If
so then we should add it to the commit message.

Otherwise,

Acked-by: Greg Rose <gvrose8192@gmail.com>

> ---
> 
> Notes:
>      v2:
>       - fix bracing style [0day robot / checkpatch]
> 
>   lib/netdev-linux.c     |  4 ++--
>   lib/netlink-notifier.c |  4 +++-
>   lib/rtnetlink.c        | 24 +++++++++++++++++++++---
>   lib/rtnetlink.h        |  2 +-
>   4 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 6be23dbee..301dd9060 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -742,7 +742,7 @@ netdev_linux_run(const struct netdev_class *netdev_class OVS_UNUSED)
>           if (!error) {
>               struct rtnetlink_change change;
>   
> -            if (rtnetlink_parse(&buf, &change)) {
> +            if (rtnetlink_parse(&buf, &change) > 0) {
>                   struct netdev *netdev_ = NULL;
>                   char dev_name[IFNAMSIZ];
>   
> @@ -6343,7 +6343,7 @@ netdev_linux_update_via_netlink(struct netdev_linux *netdev)
>           return error;
>       }
>   
> -    if (rtnetlink_parse(reply, change)
> +    if (rtnetlink_parse(reply, change) > 0
>           && change->nlmsg_type == RTM_NEWLINK) {
>           bool changed = false;
>           error = 0;
> diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
> index dfecb9778..ab5a84abb 100644
> --- a/lib/netlink-notifier.c
> +++ b/lib/netlink-notifier.c
> @@ -189,8 +189,10 @@ nln_run(struct nln *nln)
>           if (!error) {
>               int group = nln->parse(&buf, nln->change);
>   
> -            if (group != 0) {
> +            if (group > 0) {
>                   nln_report(nln, nln->change, group);
> +            } else if (group == 0) {
> +                /* ignore some events */
>               } else {
>                   VLOG_WARN_RL(&rl, "unexpected netlink message contents");
>                   nln_report(nln, NULL, 0);
> diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
> index 125802925..316524c0f 100644
> --- a/lib/rtnetlink.c
> +++ b/lib/rtnetlink.c
> @@ -82,7 +82,7 @@ rtnetlink_parse_link_info(const struct nlattr *nla,
>   /* Parses a rtnetlink message 'buf' into 'change'.  If 'buf' is unparseable,
>    * leaves 'change' untouched and returns false.  Otherwise, populates 'change'
>    * and returns true. */
> -bool
> +int
>   rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>   {
>       const struct nlmsghdr *nlmsg = buf->data;
> @@ -99,6 +99,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>               [IFLA_MTU]    = { .type = NL_A_U32,    .optional = true },
>               [IFLA_ADDRESS] = { .type = NL_A_UNSPEC, .optional = true },
>               [IFLA_LINKINFO] = { .type = NL_A_NESTED, .optional = true },
> +            [IFLA_WIRELESS] = { .type = NL_A_UNSPEC, .optional = true },
>           };
>   
>           struct nlattr *attrs[ARRAY_SIZE(policy)];
> @@ -111,6 +112,23 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>   
>               ifinfo = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifinfo);
>   
> +            /* Wireless events can be spammy and cause a
> +             * lot of unnecessary churn and CPU load in
> +             * ovs-vswitchd. The best way to filter them out
> +             * is to rely on the IFLA_WIRELESS and
> +             * ifi_change. As per rtnetlink_ifinfo_prep() in
> +             * the kernel, the ifi_change = 0. That combined
> +             * with the fact wireless events never really
> +             * change interface state (as far as core
> +             * networking is concerned) they can be ignored
> +             * by ovs-vswitchd. It doesn't understand
> +             * wireless extensions anyway and has no way of
> +             * presenting these bits into ovsdb.
> +             */
> +            if (attrs[IFLA_WIRELESS] && ifinfo->ifi_change == 0) {
> +                return RTNLGRP_NONE;
> +            }
> +
>               change->nlmsg_type     = nlmsg->nlmsg_type;
>               change->if_index       = ifinfo->ifi_index;
>               change->ifname         = nl_attr_get_string(attrs[IFLA_IFNAME]);
> @@ -165,14 +183,14 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>           }
>       }
>   
> -    return parsed;
> +    return parsed ? RTNLGRP_LINK : -1;
>   }
>   
>   /* Return RTNLGRP_LINK on success, 0 on parse error. */
>   static int
>   rtnetlink_parse_cb(struct ofpbuf *buf, void *change)
>   {
> -    return rtnetlink_parse(buf, change) ? RTNLGRP_LINK : 0;
> +    return rtnetlink_parse(buf, change);
>   }
>   
>   /* Registers 'cb' to be called with auxiliary data 'aux' with network device
> diff --git a/lib/rtnetlink.h b/lib/rtnetlink.h
> index b6ddb4bd1..23921a63b 100644
> --- a/lib/rtnetlink.h
> +++ b/lib/rtnetlink.h
> @@ -65,7 +65,7 @@ void rtnetlink_notify_func(const struct rtnetlink_change *change,
>   
>   bool rtnetlink_type_is_rtnlgrp_link(uint16_t type);
>   bool rtnetlink_type_is_rtnlgrp_addr(uint16_t type);
> -bool rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
> +int rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
>   struct nln_notifier *
>   rtnetlink_notifier_create(rtnetlink_notify_func *, void *aux);
>   void rtnetlink_notifier_destroy(struct nln_notifier *);
>
Michal Kazior Feb. 9, 2021, 9:32 a.m. UTC | #2
On Tue, 9 Feb 2021 at 00:58, Gregory Rose <gvrose8192@gmail.com> wrote:
> On 1/14/2021 1:09 AM, Michal Kazior wrote:
> > From: Michal Kazior <michal@plume.com>
[...]
>
> Hi Michal,
>
> The patch looks fine to me.  Applies cleanly to master
> and did not cause any regressions in 'make check'.
>
> I'm curious since I don't ever use OVS with wireless ports
> if there is somewhere this issue has been reported.  If
> so then we should add it to the commit message.

Hi Greg,

I'm unaware of any reports of this sort and I wouldn't be surprised to
be the first one. Given my experience - and the fact this has been
sitting on the mailing list for almost 2 months with no comments now -
I imagine not many people tried mixing OVS with wext drivers,
especially vendor drivers.


Michał
Ilya Maximets March 1, 2021, 7:40 p.m. UTC | #3
On 1/14/21 10:09 AM, Michal Kazior wrote:
> From: Michal Kazior <michal@plume.com>
> 
> Some older wireless drivers - ones relying on the
> old and long deprecated wireless extension ioctl
> system - can generate quite a bit of IFLA_WIRELESS
> events depending on their configuration and
> runtime conditions. These are delivered as
> RTNLGRP_LINK via RTM_NEWLINK messages.
> 
> These tend to be relatively easily identifiable
> because they report the change mask being 0. This
> isn't guaranteed but in practice it shouldn't be a
> problem. None of the wireless events that I ever
> observed actually carry any unique information
> about netdev states that ovs-vswitchd is
> interested in. Hence ignoring these shouldn't
> cause any problems.
> 
> These events can be responsible for a significant
> CPU churn as ovs-vswitchd attempts to do plenty of
> work for each and every netlink message regardless
> of what that message carries. On low-end devices
> such as consumer-grade routers these can lead to a
> lot of CPU cycles being wasted, adding up to heat
> output and reducing performance.
> 
> It could be argued that wireless drivers in
> question should be fixed, but that isn't exactly a
> trivial thing to do. Patching ovs seems far more
> viable while still making sense.
> 
> This change requires a slight tweak to the netlink
> abstraction code so that the events can be
> discarded as soon as possible before getting a
> chance to cause the churn.
> 
> Signed-off-by: Michal Kazior <michal@plume.com>

Hi, Michal.  Thanks for working on this!
The idea seems reasonable to me.  Some comments for the implementation
inline.

Bets regards, Ilya Maximets.

> ---
> 
> Notes:
>     v2:
>      - fix bracing style [0day robot / checkpatch]
> 
>  lib/netdev-linux.c     |  4 ++--
>  lib/netlink-notifier.c |  4 +++-
>  lib/rtnetlink.c        | 24 +++++++++++++++++++++---
>  lib/rtnetlink.h        |  2 +-
>  4 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 6be23dbee..301dd9060 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -742,7 +742,7 @@ netdev_linux_run(const struct netdev_class *netdev_class OVS_UNUSED)
>          if (!error) {
>              struct rtnetlink_change change;
>  
> -            if (rtnetlink_parse(&buf, &change)) {
> +            if (rtnetlink_parse(&buf, &change) > 0) {
>                  struct netdev *netdev_ = NULL;
>                  char dev_name[IFNAMSIZ];
>  
> @@ -6343,7 +6343,7 @@ netdev_linux_update_via_netlink(struct netdev_linux *netdev)
>          return error;
>      }
>  
> -    if (rtnetlink_parse(reply, change)
> +    if (rtnetlink_parse(reply, change) > 0
>          && change->nlmsg_type == RTM_NEWLINK) {
>          bool changed = false;
>          error = 0;
> diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
> index dfecb9778..ab5a84abb 100644
> --- a/lib/netlink-notifier.c
> +++ b/lib/netlink-notifier.c
> @@ -189,8 +189,10 @@ nln_run(struct nln *nln)
>          if (!error) {
>              int group = nln->parse(&buf, nln->change);
>  
> -            if (group != 0) {
> +            if (group > 0) {
>                  nln_report(nln, nln->change, group);
> +            } else if (group == 0) {
> +                /* ignore some events */
>              } else {
>                  VLOG_WARN_RL(&rl, "unexpected netlink message contents");
>                  nln_report(nln, NULL, 0);
> diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
> index 125802925..316524c0f 100644
> --- a/lib/rtnetlink.c
> +++ b/lib/rtnetlink.c
> @@ -82,7 +82,7 @@ rtnetlink_parse_link_info(const struct nlattr *nla,
>  /* Parses a rtnetlink message 'buf' into 'change'.  If 'buf' is unparseable,
>   * leaves 'change' untouched and returns false.  Otherwise, populates 'change'
>   * and returns true. */

Comment above is no longer correct with this change.
In general, changing the semantics of a 'parse' method looks
a bit tricky.  Maybe we can do this a bit differently?
I'd suggest to keep the 'rtnetlink_parse' almost as is, but
add some extra flag to 'struct rtnetlink_change', that will
be set for change events that are not interesting for OVS.

Something like 'bool irrelevant;'  'rtnetlink_parse' will set
it to 'true' for wireless events with a fair comment that
"wireless events never really change interface state".
So, 'nln_run' will only report events if this flag is not
set and netdev-linux will check for:
  if (rtnetlink_parse(&buf, &change) && !change->irrelevant)

What do you think?

> -bool
> +int
>  rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>  {
>      const struct nlmsghdr *nlmsg = buf->data;
> @@ -99,6 +99,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>              [IFLA_MTU]    = { .type = NL_A_U32,    .optional = true },
>              [IFLA_ADDRESS] = { .type = NL_A_UNSPEC, .optional = true },
>              [IFLA_LINKINFO] = { .type = NL_A_NESTED, .optional = true },
> +            [IFLA_WIRELESS] = { .type = NL_A_UNSPEC, .optional = true },
>          };
>  
>          struct nlattr *attrs[ARRAY_SIZE(policy)];
> @@ -111,6 +112,23 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>  
>              ifinfo = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifinfo);
>  
> +            /* Wireless events can be spammy and cause a
> +             * lot of unnecessary churn and CPU load in
> +             * ovs-vswitchd. The best way to filter them out
> +             * is to rely on the IFLA_WIRELESS and
> +             * ifi_change. As per rtnetlink_ifinfo_prep() in
> +             * the kernel, the ifi_change = 0. That combined
> +             * with the fact wireless events never really
> +             * change interface state (as far as core
> +             * networking is concerned) they can be ignored
> +             * by ovs-vswitchd. It doesn't understand
> +             * wireless extensions anyway and has no way of
> +             * presenting these bits into ovsdb.
> +             */
> +            if (attrs[IFLA_WIRELESS] && ifinfo->ifi_change == 0) {
> +                return RTNLGRP_NONE;
> +            }
> +
>              change->nlmsg_type     = nlmsg->nlmsg_type;
>              change->if_index       = ifinfo->ifi_index;
>              change->ifname         = nl_attr_get_string(attrs[IFLA_IFNAME]);
> @@ -165,14 +183,14 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>          }
>      }
>  
> -    return parsed;
> +    return parsed ? RTNLGRP_LINK : -1;
>  }
>  
>  /* Return RTNLGRP_LINK on success, 0 on parse error. */
>  static int
>  rtnetlink_parse_cb(struct ofpbuf *buf, void *change)
>  {
> -    return rtnetlink_parse(buf, change) ? RTNLGRP_LINK : 0;
> +    return rtnetlink_parse(buf, change);
>  }
>  
>  /* Registers 'cb' to be called with auxiliary data 'aux' with network device
> diff --git a/lib/rtnetlink.h b/lib/rtnetlink.h
> index b6ddb4bd1..23921a63b 100644
> --- a/lib/rtnetlink.h
> +++ b/lib/rtnetlink.h
> @@ -65,7 +65,7 @@ void rtnetlink_notify_func(const struct rtnetlink_change *change,
>  
>  bool rtnetlink_type_is_rtnlgrp_link(uint16_t type);
>  bool rtnetlink_type_is_rtnlgrp_addr(uint16_t type);
> -bool rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
> +int rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
>  struct nln_notifier *
>  rtnetlink_notifier_create(rtnetlink_notify_func *, void *aux);
>  void rtnetlink_notifier_destroy(struct nln_notifier *);
>
Michal Kazior March 2, 2021, 9:19 a.m. UTC | #4
On Mon, Mar 1, 2021 at 8:40 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 1/14/21 10:09 AM, Michal Kazior wrote:
> > From: Michal Kazior <michal@plume.com>
[...]
> Hi, Michal.  Thanks for working on this!
> The idea seems reasonable to me.  Some comments for the implementation
> inline.
[...]
> > diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
> > index 125802925..316524c0f 100644
> > --- a/lib/rtnetlink.c
> > +++ b/lib/rtnetlink.c
> > @@ -82,7 +82,7 @@ rtnetlink_parse_link_info(const struct nlattr *nla,
> >  /* Parses a rtnetlink message 'buf' into 'change'.  If 'buf' is unparseable,
> >   * leaves 'change' untouched and returns false.  Otherwise, populates 'change'
> >   * and returns true. */
>
> Comment above is no longer correct with this change.
> In general, changing the semantics of a 'parse' method looks
> a bit tricky.  Maybe we can do this a bit differently?
> I'd suggest to keep the 'rtnetlink_parse' almost as is, but
> add some extra flag to 'struct rtnetlink_change', that will
> be set for change events that are not interesting for OVS.
>
> Something like 'bool irrelevant;'  'rtnetlink_parse' will set
> it to 'true' for wireless events with a fair comment that
> "wireless events never really change interface state".
> So, 'nln_run' will only report events if this flag is not
> set and netdev-linux will check for:
>   if (rtnetlink_parse(&buf, &change) && !change->irrelevant)
>
> What do you think?

It does sound reasonable. I think I did consider that back when I was
cooking this but apparently I discarded the idea for some reason that
I no longer remember. I'll give it a try.


Michał
diff mbox series

Patch

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 6be23dbee..301dd9060 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -742,7 +742,7 @@  netdev_linux_run(const struct netdev_class *netdev_class OVS_UNUSED)
         if (!error) {
             struct rtnetlink_change change;
 
-            if (rtnetlink_parse(&buf, &change)) {
+            if (rtnetlink_parse(&buf, &change) > 0) {
                 struct netdev *netdev_ = NULL;
                 char dev_name[IFNAMSIZ];
 
@@ -6343,7 +6343,7 @@  netdev_linux_update_via_netlink(struct netdev_linux *netdev)
         return error;
     }
 
-    if (rtnetlink_parse(reply, change)
+    if (rtnetlink_parse(reply, change) > 0
         && change->nlmsg_type == RTM_NEWLINK) {
         bool changed = false;
         error = 0;
diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
index dfecb9778..ab5a84abb 100644
--- a/lib/netlink-notifier.c
+++ b/lib/netlink-notifier.c
@@ -189,8 +189,10 @@  nln_run(struct nln *nln)
         if (!error) {
             int group = nln->parse(&buf, nln->change);
 
-            if (group != 0) {
+            if (group > 0) {
                 nln_report(nln, nln->change, group);
+            } else if (group == 0) {
+                /* ignore some events */
             } else {
                 VLOG_WARN_RL(&rl, "unexpected netlink message contents");
                 nln_report(nln, NULL, 0);
diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
index 125802925..316524c0f 100644
--- a/lib/rtnetlink.c
+++ b/lib/rtnetlink.c
@@ -82,7 +82,7 @@  rtnetlink_parse_link_info(const struct nlattr *nla,
 /* Parses a rtnetlink message 'buf' into 'change'.  If 'buf' is unparseable,
  * leaves 'change' untouched and returns false.  Otherwise, populates 'change'
  * and returns true. */
-bool
+int
 rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
 {
     const struct nlmsghdr *nlmsg = buf->data;
@@ -99,6 +99,7 @@  rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
             [IFLA_MTU]    = { .type = NL_A_U32,    .optional = true },
             [IFLA_ADDRESS] = { .type = NL_A_UNSPEC, .optional = true },
             [IFLA_LINKINFO] = { .type = NL_A_NESTED, .optional = true },
+            [IFLA_WIRELESS] = { .type = NL_A_UNSPEC, .optional = true },
         };
 
         struct nlattr *attrs[ARRAY_SIZE(policy)];
@@ -111,6 +112,23 @@  rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
 
             ifinfo = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifinfo);
 
+            /* Wireless events can be spammy and cause a
+             * lot of unnecessary churn and CPU load in
+             * ovs-vswitchd. The best way to filter them out
+             * is to rely on the IFLA_WIRELESS and
+             * ifi_change. As per rtnetlink_ifinfo_prep() in
+             * the kernel, the ifi_change = 0. That combined
+             * with the fact wireless events never really
+             * change interface state (as far as core
+             * networking is concerned) they can be ignored
+             * by ovs-vswitchd. It doesn't understand
+             * wireless extensions anyway and has no way of
+             * presenting these bits into ovsdb.
+             */
+            if (attrs[IFLA_WIRELESS] && ifinfo->ifi_change == 0) {
+                return RTNLGRP_NONE;
+            }
+
             change->nlmsg_type     = nlmsg->nlmsg_type;
             change->if_index       = ifinfo->ifi_index;
             change->ifname         = nl_attr_get_string(attrs[IFLA_IFNAME]);
@@ -165,14 +183,14 @@  rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
         }
     }
 
-    return parsed;
+    return parsed ? RTNLGRP_LINK : -1;
 }
 
 /* Return RTNLGRP_LINK on success, 0 on parse error. */
 static int
 rtnetlink_parse_cb(struct ofpbuf *buf, void *change)
 {
-    return rtnetlink_parse(buf, change) ? RTNLGRP_LINK : 0;
+    return rtnetlink_parse(buf, change);
 }
 
 /* Registers 'cb' to be called with auxiliary data 'aux' with network device
diff --git a/lib/rtnetlink.h b/lib/rtnetlink.h
index b6ddb4bd1..23921a63b 100644
--- a/lib/rtnetlink.h
+++ b/lib/rtnetlink.h
@@ -65,7 +65,7 @@  void rtnetlink_notify_func(const struct rtnetlink_change *change,
 
 bool rtnetlink_type_is_rtnlgrp_link(uint16_t type);
 bool rtnetlink_type_is_rtnlgrp_addr(uint16_t type);
-bool rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
+int rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
 struct nln_notifier *
 rtnetlink_notifier_create(rtnetlink_notify_func *, void *aux);
 void rtnetlink_notifier_destroy(struct nln_notifier *);