* [PATCH net-next] bonding: make global bonding stats more reliable
@ 2014-09-25 13:16 Andy Gospodarek
2014-09-26 8:52 ` Nikolay Aleksandrov
0 siblings, 1 reply; 5+ messages in thread
From: Andy Gospodarek @ 2014-09-25 13:16 UTC (permalink / raw)
To: netdev; +Cc: j.vosburgh, vfalico, nikolay
As the code stands today, bonding stats are based simply on the stats
from the member interfaces. If a member was to be removed from a bond,
the stats would instantly drop. This would be confusing to an admin
would would suddonly see interface stats drop while traffic is still
flowing.
In addition to preventing the stats drops mentioned above, new members
will now be added to the bond and only traffic received after the member
was added to the bond will be counted as part of bonding stats.
Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
---
drivers/net/bonding/bond_main.c | 63 +++++++++++++++++++++++++++--------------
drivers/net/bonding/bonding.h | 3 ++
2 files changed, 44 insertions(+), 22 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5390475..4543c74 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1149,11 +1149,19 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
if (!slave)
return NULL;
+ slave->slave_stats = kzalloc(sizeof(struct rtnl_link_stats64),
+ GFP_KERNEL);
+ if (!slave->slave_stats) {
+ kfree(slave);
+ return NULL;
+ }
+
if (BOND_MODE(bond) == BOND_MODE_8023AD) {
SLAVE_AD_INFO(slave) = kzalloc(sizeof(struct ad_slave_info),
GFP_KERNEL);
if (!SLAVE_AD_INFO(slave)) {
kfree(slave);
+ kfree(slave->slave_stats);
return NULL;
}
}
@@ -1167,6 +1175,7 @@ static void bond_free_slave(struct slave *slave)
if (BOND_MODE(bond) == BOND_MODE_8023AD)
kfree(SLAVE_AD_INFO(slave));
+ kfree(slave->slave_stats);
kfree(slave);
}
@@ -1344,6 +1353,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
}
slave_dev->priv_flags |= IFF_BONDING;
+ /* initialize slave stats */
+ dev_get_stats(new_slave->dev, new_slave->slave_stats);
if (bond_is_lb(bond)) {
/* bond_alb_init_slave() must be called before all other stages since
@@ -3085,38 +3096,43 @@ static struct rtnl_link_stats64 *bond_get_stats(struct net_device *bond_dev,
struct list_head *iter;
struct slave *slave;
- memset(stats, 0, sizeof(*stats));
+ memcpy(stats, bond->bond_stats, sizeof(*stats));
bond_for_each_slave(bond, slave, iter) {
const struct rtnl_link_stats64 *sstats =
dev_get_stats(slave->dev, &temp);
+ struct rtnl_link_stats64 *pstats = slave->slave_stats;
+
+ stats->rx_packets += sstats->rx_packets - pstats->rx_packets;
+ stats->rx_bytes += sstats->rx_bytes - pstats->rx_bytes;
+ stats->rx_errors += sstats->rx_errors - pstats->rx_errors;
+ stats->rx_dropped += sstats->rx_dropped - pstats->rx_dropped;
- stats->rx_packets += sstats->rx_packets;
- stats->rx_bytes += sstats->rx_bytes;
- stats->rx_errors += sstats->rx_errors;
- stats->rx_dropped += sstats->rx_dropped;
+ stats->tx_packets += sstats->tx_packets - pstats->tx_packets;;
+ stats->tx_bytes += sstats->tx_bytes - pstats->tx_bytes;
+ stats->tx_errors += sstats->tx_errors - pstats->tx_errors;
+ stats->tx_dropped += sstats->tx_dropped - pstats->tx_dropped;
- stats->tx_packets += sstats->tx_packets;
- stats->tx_bytes += sstats->tx_bytes;
- stats->tx_errors += sstats->tx_errors;
- stats->tx_dropped += sstats->tx_dropped;
+ stats->multicast += sstats->multicast - pstats->multicast;
+ stats->collisions += sstats->collisions - pstats->collisions;
- stats->multicast += sstats->multicast;
- stats->collisions += sstats->collisions;
+ stats->rx_length_errors += sstats->rx_length_errors - pstats->rx_length_errors;
+ stats->rx_over_errors += sstats->rx_over_errors - pstats->rx_over_errors;
+ stats->rx_crc_errors += sstats->rx_crc_errors - pstats->rx_crc_errors;
+ stats->rx_frame_errors += sstats->rx_frame_errors - pstats->rx_frame_errors;
+ stats->rx_fifo_errors += sstats->rx_fifo_errors - pstats->rx_fifo_errors;
+ stats->rx_missed_errors += sstats->rx_missed_errors - pstats->rx_missed_errors;
- stats->rx_length_errors += sstats->rx_length_errors;
- stats->rx_over_errors += sstats->rx_over_errors;
- stats->rx_crc_errors += sstats->rx_crc_errors;
- stats->rx_frame_errors += sstats->rx_frame_errors;
- stats->rx_fifo_errors += sstats->rx_fifo_errors;
- stats->rx_missed_errors += sstats->rx_missed_errors;
+ stats->tx_aborted_errors += sstats->tx_aborted_errors - pstats->tx_aborted_errors;
+ stats->tx_carrier_errors += sstats->tx_carrier_errors - pstats->tx_carrier_errors;
+ stats->tx_fifo_errors += sstats->tx_fifo_errors - pstats->tx_fifo_errors;
+ stats->tx_heartbeat_errors += sstats->tx_heartbeat_errors - pstats->tx_heartbeat_errors;
+ stats->tx_window_errors += sstats->tx_window_errors - pstats->tx_window_errors;
- stats->tx_aborted_errors += sstats->tx_aborted_errors;
- stats->tx_carrier_errors += sstats->tx_carrier_errors;
- stats->tx_fifo_errors += sstats->tx_fifo_errors;
- stats->tx_heartbeat_errors += sstats->tx_heartbeat_errors;
- stats->tx_window_errors += sstats->tx_window_errors;
+ /* save off the slave stats for the next run */
+ memcpy(pstats, sstats, sizeof(*sstats));
}
+ memcpy(bond->bond_stats, stats, sizeof(*stats));
return stats;
}
@@ -4258,6 +4274,9 @@ static int bond_init(struct net_device *bond_dev)
bond_dev->addr_assign_type == NET_ADDR_PERM)
eth_hw_addr_random(bond_dev);
+ /* initialize persistent stats for the bond */
+ bond->bond_stats = kzalloc(sizeof(struct rtnl_link_stats64),
+ GFP_ATOMIC);
return 0;
}
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 6140bf0..fe25265 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -24,6 +24,7 @@
#include <linux/inetdevice.h>
#include <linux/etherdevice.h>
#include <linux/reciprocal_div.h>
+#include <linux/if_link.h>
#include "bond_3ad.h"
#include "bond_alb.h"
@@ -175,6 +176,7 @@ struct slave {
struct netpoll *np;
#endif
struct kobject kobj;
+ struct rtnl_link_stats64 *slave_stats;
};
/*
@@ -224,6 +226,7 @@ struct bonding {
/* debugging support via debugfs */
struct dentry *debug_dir;
#endif /* CONFIG_DEBUG_FS */
+ struct rtnl_link_stats64 *bond_stats;
};
#define bond_slave_get_rcu(dev) \
--
1.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next] bonding: make global bonding stats more reliable
2014-09-25 13:16 [PATCH net-next] bonding: make global bonding stats more reliable Andy Gospodarek
@ 2014-09-26 8:52 ` Nikolay Aleksandrov
2014-09-26 13:26 ` Andy Gospodarek
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay Aleksandrov @ 2014-09-26 8:52 UTC (permalink / raw)
To: Andy Gospodarek, netdev; +Cc: j.vosburgh, vfalico
On 25/09/14 15:16, Andy Gospodarek wrote:
> As the code stands today, bonding stats are based simply on the stats
> from the member interfaces. If a member was to be removed from a bond,
> the stats would instantly drop. This would be confusing to an admin
> would would suddonly see interface stats drop while traffic is still
> flowing.
>
> In addition to preventing the stats drops mentioned above, new members
> will now be added to the bond and only traffic received after the member
> was added to the bond will be counted as part of bonding stats.
>
> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
> ---
Hi Andy,
> drivers/net/bonding/bond_main.c | 63 +++++++++++++++++++++++++++--------------
> drivers/net/bonding/bonding.h | 3 ++
> 2 files changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 5390475..4543c74 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1149,11 +1149,19 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
> if (!slave)
> return NULL;
>
> + slave->slave_stats = kzalloc(sizeof(struct rtnl_link_stats64),
> + GFP_KERNEL);
> + if (!slave->slave_stats) {
> + kfree(slave);
> + return NULL;
> + }
> +
> if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> SLAVE_AD_INFO(slave) = kzalloc(sizeof(struct ad_slave_info),
> GFP_KERNEL);
> if (!SLAVE_AD_INFO(slave)) {
> kfree(slave);
> + kfree(slave->slave_stats);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Wrong order. 1. slave_stats, 2. slave.
> return NULL;
> }
> }
> @@ -1167,6 +1175,7 @@ static void bond_free_slave(struct slave *slave)
> if (BOND_MODE(bond) == BOND_MODE_8023AD)
> kfree(SLAVE_AD_INFO(slave));
>
> + kfree(slave->slave_stats);
> kfree(slave);
> }
>
> @@ -1344,6 +1353,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> }
>
> slave_dev->priv_flags |= IFF_BONDING;
> + /* initialize slave stats */
> + dev_get_stats(new_slave->dev, new_slave->slave_stats);
>
> if (bond_is_lb(bond)) {
> /* bond_alb_init_slave() must be called before all other stages since
> @@ -3085,38 +3096,43 @@ static struct rtnl_link_stats64 *bond_get_stats(struct net_device *bond_dev,
> struct list_head *iter;
> struct slave *slave;
>
> - memset(stats, 0, sizeof(*stats));
> + memcpy(stats, bond->bond_stats, sizeof(*stats));
>
> bond_for_each_slave(bond, slave, iter) {
> const struct rtnl_link_stats64 *sstats =
> dev_get_stats(slave->dev, &temp);
> + struct rtnl_link_stats64 *pstats = slave->slave_stats;
> +
> + stats->rx_packets += sstats->rx_packets - pstats->rx_packets;
> + stats->rx_bytes += sstats->rx_bytes - pstats->rx_bytes;
> + stats->rx_errors += sstats->rx_errors - pstats->rx_errors;
> + stats->rx_dropped += sstats->rx_dropped - pstats->rx_dropped;
>
> - stats->rx_packets += sstats->rx_packets;
> - stats->rx_bytes += sstats->rx_bytes;
> - stats->rx_errors += sstats->rx_errors;
> - stats->rx_dropped += sstats->rx_dropped;
> + stats->tx_packets += sstats->tx_packets - pstats->tx_packets;;
> + stats->tx_bytes += sstats->tx_bytes - pstats->tx_bytes;
> + stats->tx_errors += sstats->tx_errors - pstats->tx_errors;
> + stats->tx_dropped += sstats->tx_dropped - pstats->tx_dropped;
>
> - stats->tx_packets += sstats->tx_packets;
> - stats->tx_bytes += sstats->tx_bytes;
> - stats->tx_errors += sstats->tx_errors;
> - stats->tx_dropped += sstats->tx_dropped;
> + stats->multicast += sstats->multicast - pstats->multicast;
> + stats->collisions += sstats->collisions - pstats->collisions;
>
> - stats->multicast += sstats->multicast;
> - stats->collisions += sstats->collisions;
> + stats->rx_length_errors += sstats->rx_length_errors - pstats->rx_length_errors;
> + stats->rx_over_errors += sstats->rx_over_errors - pstats->rx_over_errors;
> + stats->rx_crc_errors += sstats->rx_crc_errors - pstats->rx_crc_errors;
> + stats->rx_frame_errors += sstats->rx_frame_errors - pstats->rx_frame_errors;
> + stats->rx_fifo_errors += sstats->rx_fifo_errors - pstats->rx_fifo_errors;
> + stats->rx_missed_errors += sstats->rx_missed_errors - pstats->rx_missed_errors;
>
> - stats->rx_length_errors += sstats->rx_length_errors;
> - stats->rx_over_errors += sstats->rx_over_errors;
> - stats->rx_crc_errors += sstats->rx_crc_errors;
> - stats->rx_frame_errors += sstats->rx_frame_errors;
> - stats->rx_fifo_errors += sstats->rx_fifo_errors;
> - stats->rx_missed_errors += sstats->rx_missed_errors;
> + stats->tx_aborted_errors += sstats->tx_aborted_errors - pstats->tx_aborted_errors;
> + stats->tx_carrier_errors += sstats->tx_carrier_errors - pstats->tx_carrier_errors;
> + stats->tx_fifo_errors += sstats->tx_fifo_errors - pstats->tx_fifo_errors;
> + stats->tx_heartbeat_errors += sstats->tx_heartbeat_errors - pstats->tx_heartbeat_errors;
> + stats->tx_window_errors += sstats->tx_window_errors - pstats->tx_window_errors;
>
> - stats->tx_aborted_errors += sstats->tx_aborted_errors;
> - stats->tx_carrier_errors += sstats->tx_carrier_errors;
> - stats->tx_fifo_errors += sstats->tx_fifo_errors;
> - stats->tx_heartbeat_errors += sstats->tx_heartbeat_errors;
> - stats->tx_window_errors += sstats->tx_window_errors;
> + /* save off the slave stats for the next run */
> + memcpy(pstats, sstats, sizeof(*sstats));
> }
> + memcpy(bond->bond_stats, stats, sizeof(*stats));
>
> return stats;
> }
> @@ -4258,6 +4274,9 @@ static int bond_init(struct net_device *bond_dev)
> bond_dev->addr_assign_type == NET_ADDR_PERM)
> eth_hw_addr_random(bond_dev);
>
> + /* initialize persistent stats for the bond */
> + bond->bond_stats = kzalloc(sizeof(struct rtnl_link_stats64),
> + GFP_ATOMIC);
^^^^^^^^^^^^^^^^^^^^^^^^
I don't think this will get freed if the bond device is destroyed.
> return 0;
> }
>
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 6140bf0..fe25265 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -24,6 +24,7 @@
> #include <linux/inetdevice.h>
> #include <linux/etherdevice.h>
> #include <linux/reciprocal_div.h>
> +#include <linux/if_link.h>
>
> #include "bond_3ad.h"
> #include "bond_alb.h"
> @@ -175,6 +176,7 @@ struct slave {
> struct netpoll *np;
> #endif
> struct kobject kobj;
> + struct rtnl_link_stats64 *slave_stats;
> };
>
> /*
> @@ -224,6 +226,7 @@ struct bonding {
> /* debugging support via debugfs */
> struct dentry *debug_dir;
> #endif /* CONFIG_DEBUG_FS */
> + struct rtnl_link_stats64 *bond_stats;
> };
>
> #define bond_slave_get_rcu(dev) \
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next] bonding: make global bonding stats more reliable
2014-09-26 8:52 ` Nikolay Aleksandrov
@ 2014-09-26 13:26 ` Andy Gospodarek
2014-09-26 13:35 ` Nikolay Aleksandrov
0 siblings, 1 reply; 5+ messages in thread
From: Andy Gospodarek @ 2014-09-26 13:26 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, j.vosburgh, vfalico
On Fri, Sep 26, 2014 at 10:52:16AM +0200, Nikolay Aleksandrov wrote:
> On 25/09/14 15:16, Andy Gospodarek wrote:
> >As the code stands today, bonding stats are based simply on the stats
> >from the member interfaces. If a member was to be removed from a bond,
> >the stats would instantly drop. This would be confusing to an admin
> >would would suddonly see interface stats drop while traffic is still
> >flowing.
> >
> >In addition to preventing the stats drops mentioned above, new members
> >will now be added to the bond and only traffic received after the member
> >was added to the bond will be counted as part of bonding stats.
> >
> >Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
> >---
> Hi Andy,
>
> > drivers/net/bonding/bond_main.c | 63 +++++++++++++++++++++++++++--------------
> > drivers/net/bonding/bonding.h | 3 ++
> > 2 files changed, 44 insertions(+), 22 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 5390475..4543c74 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1149,11 +1149,19 @@ static struct slave *bond_alloc_slave(struct bonding *bond)
> > if (!slave)
> > return NULL;
> >
> >+ slave->slave_stats = kzalloc(sizeof(struct rtnl_link_stats64),
> >+ GFP_KERNEL);
> >+ if (!slave->slave_stats) {
> >+ kfree(slave);
> >+ return NULL;
> >+ }
> >+
> > if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> > SLAVE_AD_INFO(slave) = kzalloc(sizeof(struct ad_slave_info),
> > GFP_KERNEL);
> > if (!SLAVE_AD_INFO(slave)) {
> > kfree(slave);
> >+ kfree(slave->slave_stats);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Wrong order. 1. slave_stats, 2. slave.
Ah yes, thanks! Clearly I didn't test the error path.
>
> > return NULL;
> > }
> > }
> >@@ -1167,6 +1175,7 @@ static void bond_free_slave(struct slave *slave)
> > if (BOND_MODE(bond) == BOND_MODE_8023AD)
> > kfree(SLAVE_AD_INFO(slave));
> >
> >+ kfree(slave->slave_stats);
> > kfree(slave);
> > }
> >
> >@@ -1344,6 +1353,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> > }
> >
> > slave_dev->priv_flags |= IFF_BONDING;
> >+ /* initialize slave stats */
> >+ dev_get_stats(new_slave->dev, new_slave->slave_stats);
> >
> > if (bond_is_lb(bond)) {
> > /* bond_alb_init_slave() must be called before all other stages since
> >@@ -3085,38 +3096,43 @@ static struct rtnl_link_stats64 *bond_get_stats(struct net_device *bond_dev,
> > struct list_head *iter;
> > struct slave *slave;
> >
> >- memset(stats, 0, sizeof(*stats));
> >+ memcpy(stats, bond->bond_stats, sizeof(*stats));
> >
> > bond_for_each_slave(bond, slave, iter) {
> > const struct rtnl_link_stats64 *sstats =
> > dev_get_stats(slave->dev, &temp);
> >+ struct rtnl_link_stats64 *pstats = slave->slave_stats;
> >+
> >+ stats->rx_packets += sstats->rx_packets - pstats->rx_packets;
> >+ stats->rx_bytes += sstats->rx_bytes - pstats->rx_bytes;
> >+ stats->rx_errors += sstats->rx_errors - pstats->rx_errors;
> >+ stats->rx_dropped += sstats->rx_dropped - pstats->rx_dropped;
> >
> >- stats->rx_packets += sstats->rx_packets;
> >- stats->rx_bytes += sstats->rx_bytes;
> >- stats->rx_errors += sstats->rx_errors;
> >- stats->rx_dropped += sstats->rx_dropped;
> >+ stats->tx_packets += sstats->tx_packets - pstats->tx_packets;;
> >+ stats->tx_bytes += sstats->tx_bytes - pstats->tx_bytes;
> >+ stats->tx_errors += sstats->tx_errors - pstats->tx_errors;
> >+ stats->tx_dropped += sstats->tx_dropped - pstats->tx_dropped;
> >
> >- stats->tx_packets += sstats->tx_packets;
> >- stats->tx_bytes += sstats->tx_bytes;
> >- stats->tx_errors += sstats->tx_errors;
> >- stats->tx_dropped += sstats->tx_dropped;
> >+ stats->multicast += sstats->multicast - pstats->multicast;
> >+ stats->collisions += sstats->collisions - pstats->collisions;
> >
> >- stats->multicast += sstats->multicast;
> >- stats->collisions += sstats->collisions;
> >+ stats->rx_length_errors += sstats->rx_length_errors - pstats->rx_length_errors;
> >+ stats->rx_over_errors += sstats->rx_over_errors - pstats->rx_over_errors;
> >+ stats->rx_crc_errors += sstats->rx_crc_errors - pstats->rx_crc_errors;
> >+ stats->rx_frame_errors += sstats->rx_frame_errors - pstats->rx_frame_errors;
> >+ stats->rx_fifo_errors += sstats->rx_fifo_errors - pstats->rx_fifo_errors;
> >+ stats->rx_missed_errors += sstats->rx_missed_errors - pstats->rx_missed_errors;
> >
> >- stats->rx_length_errors += sstats->rx_length_errors;
> >- stats->rx_over_errors += sstats->rx_over_errors;
> >- stats->rx_crc_errors += sstats->rx_crc_errors;
> >- stats->rx_frame_errors += sstats->rx_frame_errors;
> >- stats->rx_fifo_errors += sstats->rx_fifo_errors;
> >- stats->rx_missed_errors += sstats->rx_missed_errors;
> >+ stats->tx_aborted_errors += sstats->tx_aborted_errors - pstats->tx_aborted_errors;
> >+ stats->tx_carrier_errors += sstats->tx_carrier_errors - pstats->tx_carrier_errors;
> >+ stats->tx_fifo_errors += sstats->tx_fifo_errors - pstats->tx_fifo_errors;
> >+ stats->tx_heartbeat_errors += sstats->tx_heartbeat_errors - pstats->tx_heartbeat_errors;
> >+ stats->tx_window_errors += sstats->tx_window_errors - pstats->tx_window_errors;
> >
> >- stats->tx_aborted_errors += sstats->tx_aborted_errors;
> >- stats->tx_carrier_errors += sstats->tx_carrier_errors;
> >- stats->tx_fifo_errors += sstats->tx_fifo_errors;
> >- stats->tx_heartbeat_errors += sstats->tx_heartbeat_errors;
> >- stats->tx_window_errors += sstats->tx_window_errors;
> >+ /* save off the slave stats for the next run */
> >+ memcpy(pstats, sstats, sizeof(*sstats));
> > }
> >+ memcpy(bond->bond_stats, stats, sizeof(*stats));
> >
> > return stats;
> > }
> >@@ -4258,6 +4274,9 @@ static int bond_init(struct net_device *bond_dev)
> > bond_dev->addr_assign_type == NET_ADDR_PERM)
> > eth_hw_addr_random(bond_dev);
> >
> >+ /* initialize persistent stats for the bond */
> >+ bond->bond_stats = kzalloc(sizeof(struct rtnl_link_stats64),
> >+ GFP_ATOMIC);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> I don't think this will get freed if the bond device is destroyed.
Another good catch. Thanks for the review. V2 incoming after some
testing.
>
> > return 0;
> > }
> >
> >diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> >index 6140bf0..fe25265 100644
> >--- a/drivers/net/bonding/bonding.h
> >+++ b/drivers/net/bonding/bonding.h
> >@@ -24,6 +24,7 @@
> > #include <linux/inetdevice.h>
> > #include <linux/etherdevice.h>
> > #include <linux/reciprocal_div.h>
> >+#include <linux/if_link.h>
> >
> > #include "bond_3ad.h"
> > #include "bond_alb.h"
> >@@ -175,6 +176,7 @@ struct slave {
> > struct netpoll *np;
> > #endif
> > struct kobject kobj;
> >+ struct rtnl_link_stats64 *slave_stats;
> > };
> >
> > /*
> >@@ -224,6 +226,7 @@ struct bonding {
> > /* debugging support via debugfs */
> > struct dentry *debug_dir;
> > #endif /* CONFIG_DEBUG_FS */
> >+ struct rtnl_link_stats64 *bond_stats;
> > };
> >
> > #define bond_slave_get_rcu(dev) \
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next] bonding: make global bonding stats more reliable
2014-09-26 13:26 ` Andy Gospodarek
@ 2014-09-26 13:35 ` Nikolay Aleksandrov
2014-09-26 13:52 ` Andy Gospodarek
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay Aleksandrov @ 2014-09-26 13:35 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: netdev, j.vosburgh, vfalico
On 26/09/14 15:26, Andy Gospodarek wrote:
> On Fri, Sep 26, 2014 at 10:52:16AM +0200, Nikolay Aleksandrov wrote:
>> On 25/09/14 15:16, Andy Gospodarek wrote:
>>> As the code stands today, bonding stats are based simply on the stats
>> >from the member interfaces. If a member was to be removed from a bond,
>>> the stats would instantly drop. This would be confusing to an admin
>>> would would suddonly see interface stats drop while traffic is still
>>> flowing.
>>>
>>> In addition to preventing the stats drops mentioned above, new members
>>> will now be added to the bond and only traffic received after the member
>>> was added to the bond will be counted as part of bonding stats.
>>>
>>> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
>>> ---
>> Hi Andy,
>>
<<<<snip>>>>
>>> @@ -4258,6 +4274,9 @@ static int bond_init(struct net_device *bond_dev)
>>> bond_dev->addr_assign_type == NET_ADDR_PERM)
>>> eth_hw_addr_random(bond_dev);
>>>
>>> + /* initialize persistent stats for the bond */
>>> + bond->bond_stats = kzalloc(sizeof(struct rtnl_link_stats64),
>>> + GFP_ATOMIC);
>> ^^^^^^^^^^^^^^^^^^^^^^^^
>> I don't think this will get freed if the bond device is destroyed.
> Another good catch. Thanks for the review. V2 incoming after some
> testing.
>
Great, one more thing I forgot to ask, why the GFP_ATOMIC here ? I believe we're
allowed to sleep in ndo_init(), and you should probably handle the case where
the bond_stats allocation fails, too.
>>
>>> return 0;
>>> }
>>>
>>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>> index 6140bf0..fe25265 100644
>>> --- a/drivers/net/bonding/bonding.h
>>> +++ b/drivers/net/bonding/bonding.h
>>> @@ -24,6 +24,7 @@
>>> #include <linux/inetdevice.h>
>>> #include <linux/etherdevice.h>
>>> #include <linux/reciprocal_div.h>
>>> +#include <linux/if_link.h>
>>>
>>> #include "bond_3ad.h"
>>> #include "bond_alb.h"
>>> @@ -175,6 +176,7 @@ struct slave {
>>> struct netpoll *np;
>>> #endif
>>> struct kobject kobj;
>>> + struct rtnl_link_stats64 *slave_stats;
>>> };
>>>
>>> /*
>>> @@ -224,6 +226,7 @@ struct bonding {
>>> /* debugging support via debugfs */
>>> struct dentry *debug_dir;
>>> #endif /* CONFIG_DEBUG_FS */
>>> + struct rtnl_link_stats64 *bond_stats;
>>> };
>>>
>>> #define bond_slave_get_rcu(dev) \
>>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next] bonding: make global bonding stats more reliable
2014-09-26 13:35 ` Nikolay Aleksandrov
@ 2014-09-26 13:52 ` Andy Gospodarek
0 siblings, 0 replies; 5+ messages in thread
From: Andy Gospodarek @ 2014-09-26 13:52 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, j.vosburgh, vfalico
On Fri, Sep 26, 2014 at 03:35:33PM +0200, Nikolay Aleksandrov wrote:
> On 26/09/14 15:26, Andy Gospodarek wrote:
> >On Fri, Sep 26, 2014 at 10:52:16AM +0200, Nikolay Aleksandrov wrote:
> >>On 25/09/14 15:16, Andy Gospodarek wrote:
> >>>As the code stands today, bonding stats are based simply on the stats
> >>>from the member interfaces. If a member was to be removed from a bond,
> >>>the stats would instantly drop. This would be confusing to an admin
> >>>would would suddonly see interface stats drop while traffic is still
> >>>flowing.
> >>>
> >>>In addition to preventing the stats drops mentioned above, new members
> >>>will now be added to the bond and only traffic received after the member
> >>>was added to the bond will be counted as part of bonding stats.
> >>>
> >>>Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
> >>>---
> >>Hi Andy,
> >>
> <<<<snip>>>>
> >>>@@ -4258,6 +4274,9 @@ static int bond_init(struct net_device *bond_dev)
> >>> bond_dev->addr_assign_type == NET_ADDR_PERM)
> >>> eth_hw_addr_random(bond_dev);
> >>>
> >>>+ /* initialize persistent stats for the bond */
> >>>+ bond->bond_stats = kzalloc(sizeof(struct rtnl_link_stats64),
> >>>+ GFP_ATOMIC);
> >>^^^^^^^^^^^^^^^^^^^^^^^^
> >>I don't think this will get freed if the bond device is destroyed.
> >Another good catch. Thanks for the review. V2 incoming after some
> >testing.
> >
> Great, one more thing I forgot to ask, why the GFP_ATOMIC here ? I believe
> we're allowed to sleep in ndo_init(), and you should probably handle the
ndo_init() can (and likely will) sleep, but since all not skb data
allocated in bonding appears to be GFP_KERNEL, I'll switch to that for
consistency.
> case where the bond_stats allocation fails, too.
Yep, on it! :)
>
> >>
> >>> return 0;
> >>> }
> >>>
> >>>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> >>>index 6140bf0..fe25265 100644
> >>>--- a/drivers/net/bonding/bonding.h
> >>>+++ b/drivers/net/bonding/bonding.h
> >>>@@ -24,6 +24,7 @@
> >>> #include <linux/inetdevice.h>
> >>> #include <linux/etherdevice.h>
> >>> #include <linux/reciprocal_div.h>
> >>>+#include <linux/if_link.h>
> >>>
> >>> #include "bond_3ad.h"
> >>> #include "bond_alb.h"
> >>>@@ -175,6 +176,7 @@ struct slave {
> >>> struct netpoll *np;
> >>> #endif
> >>> struct kobject kobj;
> >>>+ struct rtnl_link_stats64 *slave_stats;
> >>> };
> >>>
> >>> /*
> >>>@@ -224,6 +226,7 @@ struct bonding {
> >>> /* debugging support via debugfs */
> >>> struct dentry *debug_dir;
> >>> #endif /* CONFIG_DEBUG_FS */
> >>>+ struct rtnl_link_stats64 *bond_stats;
> >>> };
> >>>
> >>> #define bond_slave_get_rcu(dev) \
> >>>
> >>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-26 13:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-25 13:16 [PATCH net-next] bonding: make global bonding stats more reliable Andy Gospodarek
2014-09-26 8:52 ` Nikolay Aleksandrov
2014-09-26 13:26 ` Andy Gospodarek
2014-09-26 13:35 ` Nikolay Aleksandrov
2014-09-26 13:52 ` Andy Gospodarek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.