* Re: [net-next.git 4/9] stmmac: add the support for PTP hw clock driver
From: Richard Cochran @ 2013-03-10 12:10 UTC (permalink / raw)
To: Giuseppe CAVALLARO; +Cc: netdev, bh74.an, ayagond, Rayagond Kokatanur
In-Reply-To: <1362653419-1047-5-git-send-email-peppe.cavallaro@st.com>
I have a few comments, below.
On Thu, Mar 07, 2013 at 11:50:14AM +0100, Giuseppe CAVALLARO wrote:
> From: Rayagond Kokatanur <rayagond@vayavyalabs.com>
>
> This patch implements PHC (ptp hardware clock) driver for stmmac
> driver to support 1588 PTP.
>
> Signed-off-by: Rayagond Kokatanur <rayagond@vayavyalabs.com>
> Hacked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
> drivers/net/ethernet/stmicro/stmmac/common.h | 4 +
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 13 ++
> .../net/ethernet/stmicro/stmmac/stmmac_hwstamp.c | 50 +++++
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 31 +++-
> drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 209 ++++++++++++++++++++
> drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.h | 2 +
> 7 files changed, 303 insertions(+), 8 deletions(-)
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
> index cc97c07..40ee3df 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -3,7 +3,7 @@ stmmac-$(CONFIG_STMMAC_RING) += ring_mode.o
> stmmac-$(CONFIG_STMMAC_CHAINED) += chain_mode.o
> stmmac-$(CONFIG_STMMAC_PLATFORM) += stmmac_platform.o
> stmmac-$(CONFIG_STMMAC_PCI) += stmmac_pci.o
> -stmmac-$(CONFIG_STMMAC_USE_HWSTAMP) += stmmac_hwstamp.o
> +stmmac-$(CONFIG_STMMAC_USE_HWSTAMP) += stmmac_hwstamp.o stmmac_ptp.o
> stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o \
> dwmac_lib.o dwmac1000_core.o dwmac1000_dma.o \
> dwmac100_core.o dwmac100_dma.o enh_desc.o norm_desc.o \
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index df7123a..a10974c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -387,6 +387,10 @@ struct stmmac_hwtimestamp {
> void (*config_sub_second_increment) (void __iomem *ioaddr);
> int (*init_systime) (void __iomem *ioaddr, u32 sec, u32 nsec);
> int (*config_addend)(void __iomem *ioaddr, u32 addend);
> + int (*adjust_systime)(void __iomem *ioaddr, u32 sec, u32 nsec,
> + int add_sub);
> + u32 (*get_systime_sec)(void __iomem *ioaddr);
> + u32 (*get_systime_nsec)(void __iomem *ioaddr);
No need here for two separate functions. Combine them into one get_systime().
> };
> #endif
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 50b5f26..2d6e2e1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -32,6 +32,10 @@
> #include <linux/pci.h>
> #include "common.h"
>
> +#ifdef CONFIG_STMMAC_USE_HWSTAMP
> +#include <linux/ptp_clock_kernel.h>
> +#endif
> +
> struct stmmac_priv {
> /* Frequently used values are kept adjacent for cache effect */
> struct dma_desc *dma_tx ____cacheline_aligned;
> @@ -98,6 +102,9 @@ struct stmmac_priv {
> int hwts_tx_en;
> int hwts_rx_en;
> unsigned int default_addend;
> + struct ptp_clock *ptp_clock;
> + struct ptp_clock_info ptp_clock_ops;
> + spinlock_t ptp_lock;
> #endif
> int atds;
> };
> @@ -111,6 +118,12 @@ extern const struct stmmac_desc_ops enh_desc_ops;
> extern const struct stmmac_desc_ops ndesc_ops;
> #ifdef CONFIG_STMMAC_USE_HWSTAMP
> extern const struct stmmac_hwtimestamp stmmac_ptp;
> +extern int stmmac_adjust_systime(void __iomem *ioaddr, u32 sec, u32 nsec,
> + int add_sub);
> +extern u32 stmmac_get_systime_sec(void __iomem *ioaddr);
> +extern u32 stmmac_get_systime_nsec(void __iomem *ioaddr);
> +extern int stmmac_ptp_register(struct stmmac_priv *priv);
> +extern void stmmac_ptp_unregister(struct stmmac_priv *priv);
> #endif
> int stmmac_freeze(struct net_device *ndev);
> int stmmac_restore(struct net_device *ndev);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwstamp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwstamp.c
> index be9e399..bec2278 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_hwstamp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_hwstamp.c
> @@ -121,9 +121,59 @@ static int stmmac_config_addend(void __iomem *ioaddr, u32 addend)
> return 0;
> }
>
> +static int stmmac_adjust_systime(void __iomem *ioaddr, u32 sec, u32 nsec,
> + int add_sub)
> +{
> + u32 value;
> + int limit;
> +
> + /* wait for previous(if any) system time adjust/update to complete */
These busy loops also need fixing, like in the other patch.
> + limit = 100;
> + while (limit--) {
> + if (!(readl(ioaddr + PTP_TCR) & PTP_TCR_TSUPDT))
> + break;
> + mdelay(10);
> + }
> + if (limit < 0)
> + return -EBUSY;
> +
> + writel(sec, ioaddr + PTP_STSUR);
> + writel(((add_sub << PTP_STNSUR_ADDSUB_SHIFT) | nsec),
> + ioaddr + PTP_STNSUR);
> + /* issue command to initialize the system time value */
> + value = readl(ioaddr + PTP_TCR);
> + value |= PTP_TCR_TSUPDT;
> + writel(value, ioaddr + PTP_TCR);
> +
> + /* wait for present system time adjust/update to complete */
> + limit = 100;
> + while (limit--) {
> + if (!(readl(ioaddr + PTP_TCR) & PTP_TCR_TSUPDT))
> + break;
> + mdelay(10);
> + }
> + if (limit < 0)
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> +static u32 stmmac_get_systime_sec(void __iomem *ioaddr)
> +{
> + return readl(ioaddr + PTP_STSR);
> +}
> +
> +static u32 stmmac_get_systime_nsec(void __iomem *ioaddr)
> +{
> + return readl(ioaddr + PTP_STNSR);
> +}
> +
> const struct stmmac_hwtimestamp stmmac_ptp = {
> .config_hw_tstamping = stmmac_config_hw_tstamping,
> .init_systime = stmmac_init_systime,
> .config_sub_second_increment = stmmac_config_sub_second_increment,
> .config_addend = stmmac_config_addend,
> + .adjust_systime = stmmac_adjust_systime,
> + .get_systime_sec = stmmac_get_systime_sec,
> + .get_systime_nsec = stmmac_get_systime_nsec,
> };
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6174f34..009abf8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -474,24 +474,37 @@ static int stmmac_hwtstamp_ioctl(struct net_device *dev, struct ifreq *ifr)
> sizeof(struct hwtstamp_config)) ? -EFAULT : 0;
> }
>
> -static void stmmac_init_ptp(struct stmmac_priv *priv)
> +static int stmmac_init_ptp(struct stmmac_priv *priv)
> {
> - if (priv->dma_cap.time_stamp)
> - pr_debug("IEEE 1588-2002 Time Stamp supported\n");
> - if (priv->dma_cap.atime_stamp)
> - pr_debug("IEEE 1588-2008 Advanced Time Stamp supported\n");
> + if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp))
> + return -EOPNOTSUPP;
> +
> + if (netif_msg_hw(priv)) {
> + if (priv->dma_cap.time_stamp)
> + pr_debug("IEEE 1588-2002 Time Stamp supported\n");
> + if (priv->dma_cap.atime_stamp)
> + pr_debug("IEEE 1588-2008 Advanced Time Stamp supported\n");
> + }
>
> priv->hw->ptp = &stmmac_ptp;
>
> priv->hwts_tx_en = 0;
> priv->hwts_rx_en = 0;
>
> + return stmmac_ptp_register(priv);
> +}
> +
> +static void stmmac_release_ptp(struct stmmac_priv *priv)
> +{
> + stmmac_ptp_unregister(priv);
> }
> +
> #else
> #define stmmac_hwtstamp_ioctl(dev, ifr) (-EOPNOTSUPP)
> #define stmmac_get_rx_hwtstamp(priv, p, skb)
> #define stmmac_get_tx_hwtstamp(priv, p, skb) 0
> -#define stmmac_init_ptp(priv)
> +#define stmmac_init_ptp(priv) 0
> +#define stmmac_release_ptp(priv)
> #endif /* CONFIG_STMMAC_USE_HWSTAMP */
>
> /**
> @@ -1301,7 +1314,9 @@ static int stmmac_open(struct net_device *dev)
>
> stmmac_mmc_setup(priv);
>
> - stmmac_init_ptp(priv);
> + ret = stmmac_init_ptp(priv);
> + if (ret)
> + pr_warn("%s: failed PTP initialisation\n", __func__);
>
> #ifdef CONFIG_STMMAC_DEBUG_FS
> ret = stmmac_init_fs(dev);
> @@ -1403,6 +1418,8 @@ static int stmmac_release(struct net_device *dev)
> #endif
> clk_disable_unprepare(priv->stmmac_clk);
>
> + stmmac_release_ptp(priv);
> +
> return 0;
> }
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> new file mode 100644
> index 0000000..53680bf
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -0,0 +1,209 @@
> +/*******************************************************************************
> + PTP 1588 clock using the STMMAC.
> +
> + Copyright (C) 2013 Vayavya Labs Pvt Ltd
> +
> + This program is free software; you can redistribute it and/or modify it
> + under the terms and conditions of the GNU General Public License,
> + version 2, as published by the Free Software Foundation.
> +
> + This program is distributed in the hope it will be useful, but WITHOUT
> + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + more details.
> +
> + You should have received a copy of the GNU General Public License along with
> + this program; if not, write to the Free Software Foundation, Inc.,
> + 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> +
> + The full GNU General Public License is included in this distribution in
> + the file called "COPYING".
> +
> + Author: Rayagond Kokatanur <rayagond@vayavyalabs.com>
> +*******************************************************************************/
> +#include "stmmac.h"
> +#include "stmmac_ptp.h"
> +
> +/**
> + * stmmac_adjust_freq
> + *
> + * @ptp: pointer to ptp_clock_info structure
> + * @ppb: desired period change in parts ber billion
> + *
> + * Description: this function will adjust the frequency of hardware clock.
> + */
> +static int stmmac_adjust_freq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> + struct stmmac_priv *priv =
> + container_of(ptp, struct stmmac_priv, ptp_clock_ops);
> + u32 diff, addend;
> + int neg_adj = 0;
> + u64 adj;
> +
> + if (ppb < 0) {
> + neg_adj = 1;
> + ppb = -ppb;
> + }
> +
> + addend = priv->default_addend;
> + adj = addend;
> + adj *= ppb;
> + /* div_u64 will divided the "adj" by "1000000000ULL"
> + * and return the quotient
> + */
No need to comment what div_u64 does. We know that already.
> + diff = div_u64(adj, 1000000000ULL);
> +
> + addend = neg_adj ? (addend - diff) : (addend + diff);
> +
> + priv->hw->ptp->config_addend(priv->ioaddr, addend);
Don't you need locking here to protect against concurrent callers of
config_addend?
> +
> + return 0;
> +}
> +
> +/**
> + * stmmac_adjust_time
> + *
> + * @ptp: pointer to ptp_clock_info structure
> + * @delta: desired change in nanoseconds
> + *
> + * Description: this function will shift/adjust the hardware clock time.
> + */
> +static int stmmac_adjust_time(struct ptp_clock_info *ptp, s64 delta)
> +{
> + struct stmmac_priv *priv =
> + container_of(ptp, struct stmmac_priv, ptp_clock_ops);
> + unsigned long flags;
> + u32 sec, nsec;
> + u32 quotient, reminder;
> + int neg_adj = 0;
> +
> + spin_lock_irqsave(&priv->ptp_lock, flags);
You have locked too much code here. The arithmetic on stack variables
is already reentrant.
> +
> + if (delta < 0) {
> + neg_adj = 1;
> + delta = -delta;
> + }
> +
> + quotient = div_u64_rem(delta, 1000000000ULL, &reminder);
> + sec = quotient;
> + nsec = reminder;
Lock here instead.
> + priv->hw->ptp->adjust_systime(priv->ioaddr, sec, nsec, neg_adj);
> +
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + return 0;
> +}
> +
> +/**
> + * stmmac_get_time
> + *
> + * @ptp: pointer to ptp_clock_info structure
> + * @ts: pointer to hold time/result
> + *
> + * Description: this function will read the current time from the
> + * hardware clock and store it in @ts.
> + */
> +static int stmmac_get_time(struct ptp_clock_info *ptp, struct timespec *ts)
> +{
> + struct stmmac_priv *priv =
> + container_of(ptp, struct stmmac_priv, ptp_clock_ops);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->ptp_lock, flags);
> +
> + ts->tv_sec = priv->hw->ptp->get_systime_sec(priv->ioaddr);
> + ts->tv_nsec = priv->hw->ptp->get_systime_nsec(priv->ioaddr);
See, use just one function instead of two.
> +
> + spin_unlock_irqrestore(&priv->ptp_lock, flags);
> +
> + return 0;
> +}
> +
> +/**
> + * stmmac_set_time
> + *
> + * @ptp: pointer to ptp_clock_info structure
> + * @ts: time value to set
> + *
> + * Description: this function will set the current time on the
> + * hardware clock.
> + */
> +static int stmmac_set_time(struct ptp_clock_info *ptp,
> + const struct timespec *ts)
> +{
> + struct stmmac_priv *priv =
> + container_of(ptp, struct stmmac_priv, ptp_clock_ops);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->ptp_lock, flags);
> +
> + priv->hw->ptp->init_systime(priv->ioaddr, ts->tv_sec, ts->tv_nsec);
> +
> + spin_unlock_irqrestore(&priv->ptp_lock, flags);
> +
> + return 0;
> +}
> +
> +static int stmmac_enable(struct ptp_clock_info *ptp,
> + struct ptp_clock_request *rq, int on)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +/* structure describing a PTP hardware clock */
> +static struct ptp_clock_info stmmac_ptp_clock_ops = {
> + .owner = THIS_MODULE,
> + .name = "stmmac_ptp_clock",
> + .max_adj = STMMAC_SYSCLOCK,
This should be the maximum adjustment supported by the clock, in parts
per billion, not the clock frequency.
> + .n_alarm = 0,
> + .n_ext_ts = 0,
> + .n_per_out = 0,
> + .pps = 0,
> + .adjfreq = stmmac_adjust_freq,
> + .adjtime = stmmac_adjust_time,
> + .gettime = stmmac_get_time,
> + .settime = stmmac_set_time,
> + .enable = stmmac_enable,
> +};
Thanks,
Richard
^ permalink raw reply
* MBIM backwards compatibility and user vs kernel policy
From: Bjørn Mork @ 2013-03-10 11:21 UTC (permalink / raw)
To: Greg Suarez; +Cc: Alexey Orishko, linux-usb, netdev
Hello,
I just realized that our current strategy for MBIM backwards
compatibility results in bad user experience by *forcing* the user to
cdc_mbim if it is enabled at build time. There are legitimate reasons for
a user to select cdc_ncm instead of cdc_mbim at runtime, and at the
moment we prevent that. The most obvious and important reason is that
the required MBIM userspace applications are not ready yet. But even in
a future where these are available, I believe we should leave the choice
up to the user.
So our current build time based decision will not do. But what will?
Is a cdc_ncm module param setting MBIM vs NCM priority OK? We could
still make the default depend on whether cdc_mbim is built. I don't
think there are any reasons to complicate this with a per-device
selection. It is really a system wide setting depending on whether MBIM
is supported by the system.
What do you think?
This problem just came up as a result of a user failing to make Linux
v3.8 work on a Lenovo Thinkpad X230 with an Ericsson H5321gw Mobile
Broadband Device, while v3.7 appeared to work fine. The H5321gw
firmware support NCM backwards compatibility, making it work fine with
cdc_ncm in v3.7. But in v3.8 we select the cdc_mbim alternate setting
instead, and ModemManager does not yet know how to handle that, making
it fail.
The user noticed the new cdc_mbim module and tried blacklisting it, as
one would find natural. But to no effect, as the problem is in the MBIM
compatibility check in cdc_ncm. There appears to be no way to force the
cdc_ncm driver in v3.8 to be used for this device if cdc_mbim is
selected at build time. Which of course is a distro choice and not
something most users will touch.
I believe we need to resolve this quickly, and I'll probably just cook up
a proposed module param patch unless someone comes up with something
better.
BTW, I am starting to hate the NCM backward compatibility. There were so
many other ways this could have been solved. Changing class depending on
the selected interface altsetting is ugly, IMHO.
Bjørn
^ permalink raw reply
* Re: [net-next (TAKE 2) 0/4] IPv6 over Firewire
From: Stephan Gatzka @ 2013-03-10 10:03 UTC (permalink / raw)
To: YOSHIFUJI Hideaki; +Cc: netdev, linux1394-devel, davem, stefanr
In-Reply-To: <51310507.7030904@gmail.com>
O.k., I rechecked all checksums send by IPv6 over FireWire from Linux
and from MacOS.
The checksums from Yoshis implementation are correct, MacOS is wrong.
What I don't understand is that using my proof of concept patch. I could
ping6 MacOSX from my Linux box. Even with CHECKSUM_UNNECESSARY MacOSX
should also throw away the packets send from Linux. Hm, I have to
recheck this.
Regards,
Stephan
^ permalink raw reply
* Re: [PATCH net-next] fix buf of assigning skb->tail to network_header
From: Eric Dumazet @ 2013-03-10 9:58 UTC (permalink / raw)
To: Hong Zhiguo; +Cc: netdev, davem, stephen, zhiguo.hong
In-Reply-To: <1362908909-45697-1-git-send-email-honkiko@gmail.com>
On Sun, 2013-03-10 at 17:48 +0800, Hong Zhiguo wrote:
> in the case of NET_SKBUFF_DATA_USES_OFFSET, direct pointer
> assignment to skb->network_header is a dangerous bug.
But skb->tail is not a pointer.
Really you need to explain more than this confusing changelog
^ permalink raw reply
* [PATCH net-next] fix buf of assigning skb->tail to network_header
From: Hong Zhiguo @ 2013-03-10 9:48 UTC (permalink / raw)
To: netdev, davem, stephen; +Cc: zhiguo.hong, Hong Zhiguo
in the case of NET_SKBUFF_DATA_USES_OFFSET, direct pointer
assignment to skb->network_header is a dangerous bug.
Signed-off-by: Hong Zhiguo <honkiko@gmail.com>
---
net/ipv4/ipmr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 5f95b3a..553409b 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -980,7 +980,7 @@ static int ipmr_cache_report(struct mr_table *mrt,
/* Copy the IP header */
- skb->network_header = skb->tail;
+ skb_set_network_header(skb, skb->tail - skb->data);
skb_put(skb, ihl);
skb_copy_to_linear_data(skb, pkt->data, ihl);
ip_hdr(skb)->protocol = 0; /* Flag to the kernel this is a route add */
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net-next] don't compare skb->network_header with skb->tail
From: Eric Dumazet @ 2013-03-10 9:48 UTC (permalink / raw)
To: Hong Zhiguo; +Cc: netdev, davem, stephen, zhiguo.hong
In-Reply-To: <1362906925-45308-1-git-send-email-honkiko@gmail.com>
On Sun, 2013-03-10 at 17:15 +0800, Hong Zhiguo wrote:
> in the case of NET_SKBUFF_DATA_USES_OFFSET(on 64-bit arch),
> skb->network_header is just offset over skb->head.
>
> Signed-off-by: Hong Zhiguo <honkiko@gmail.com>
> ---
> net/core/dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 90cee5b..378e0b9 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1705,7 +1705,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
> skb_reset_mac_header(skb2);
>
> if (skb_network_header(skb2) < skb2->data ||
> - skb2->network_header > skb2->tail) {
> + skb_network_header(skb2) > skb2->tail) {
> net_crit_ratelimited("protocol %04x is buggy, dev %s\n",
> ntohs(skb2->protocol),
> dev->name);
Sorry I dont understand this patch.
Could you elaborate ?
^ permalink raw reply
* Re: [PATCH 0/3] net: netlink info leak fixes
From: David Miller @ 2013-03-10 9:20 UTC (permalink / raw)
To: minipli; +Cc: netdev
In-Reply-To: <1362844341-12591-1-git-send-email-minipli@googlemail.com>
From: Mathias Krause <minipli@googlemail.com>
Date: Sat, 9 Mar 2013 16:52:18 +0100
> a few more info leak fixes -- this time in rtnl.
>
> Please apply!
All applied and queued up for -stable, thanks!
^ permalink raw reply
* Re: [PATCHv2 net-next 01/15] net: add skb_dst_set_unref
From: David Miller @ 2013-03-10 9:17 UTC (permalink / raw)
To: ja; +Cc: horms, lvs-devel, netdev
In-Reply-To: <1362863815-5898-2-git-send-email-ja@ssi.bg>
From: Julian Anastasov <ja@ssi.bg>
Date: Sat, 9 Mar 2013 23:16:41 +0200
> skb_dst_set_unref will use noref version even for
> DST_NOCACHE entries because DST_NOCACHE means dst is not
> cached in routing structures, still dst could be cached
> by routing users and used to produce noref instances.
>
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
I'm fine with this approach, but I think the name of this
interface could be better.
In fact you could do something like:
1) Rename skb_dst_set_noref() to __skb_dst_set_noref() and add
a new "bool force" parameter. DST_NOCACHE check is overriden
when 'force' is true.
2) skb_dst_set_noref() is an inline that passes 'force' as false.
3) New interface skb_dst_set_noref_force() passes 'force' as true
and will be used by your IPVS changes.
Then all of the RCU checks etc. happen in one shared function.
^ permalink raw reply
* [PATCH net-next] don't compare skb->network_header with skb->tail
From: Hong Zhiguo @ 2013-03-10 9:15 UTC (permalink / raw)
To: netdev, davem, stephen; +Cc: zhiguo.hong, Hong Zhiguo
in the case of NET_SKBUFF_DATA_USES_OFFSET(on 64-bit arch),
skb->network_header is just offset over skb->head.
Signed-off-by: Hong Zhiguo <honkiko@gmail.com>
---
net/core/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 90cee5b..378e0b9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1705,7 +1705,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
skb_reset_mac_header(skb2);
if (skb_network_header(skb2) < skb2->data ||
- skb2->network_header > skb2->tail) {
+ skb_network_header(skb2) > skb2->tail) {
net_crit_ratelimited("protocol %04x is buggy, dev %s\n",
ntohs(skb2->protocol),
dev->name);
--
1.7.10.4
^ permalink raw reply related
* [Patch net-next] ipv6: introduce ip6tunnel_xmit() helper
From: Cong Wang @ 2013-03-10 9:00 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, Pravin B Shelar, David S. Miller, Cong Wang
From: Cong Wang <xiyou.wangcong@gmail.com>
Similar to iptunnel_xmit(), group these operations into a
helper function.
This by the way fixes the missing u64_stats_update_begin()
and u64_stats_update_end() for 32 bit arch.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
include/net/ip6_tunnel.h | 20 ++++++++++++++++++++
net/ipv6/ip6_gre.c | 17 +----------------
net/ipv6/ip6_tunnel.c | 15 +--------------
3 files changed, 22 insertions(+), 30 deletions(-)
diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
index e03047f..ebdef7f 100644
--- a/include/net/ip6_tunnel.h
+++ b/include/net/ip6_tunnel.h
@@ -68,4 +68,24 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw);
__u32 ip6_tnl_get_cap(struct ip6_tnl *t, const struct in6_addr *laddr,
const struct in6_addr *raddr);
+static inline void ip6tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+ struct net_device_stats *stats = &dev->stats;
+ int pkt_len, err;
+
+ nf_reset(skb);
+ pkt_len = skb->len;
+ err = ip6_local_out(skb);
+
+ if (net_xmit_eval(err) == 0) {
+ struct pcpu_tstats *tstats = this_cpu_ptr(dev->tstats);
+ u64_stats_update_begin(&tstats->syncp);
+ tstats->tx_bytes += pkt_len;
+ tstats->tx_packets++;
+ u64_stats_update_end(&tstats->syncp);
+ } else {
+ stats->tx_errors++;
+ stats->tx_aborted_errors++;
+ }
+}
#endif
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index e4efffe..6a6ba73 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -667,7 +667,6 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
struct net_device_stats *stats = &tunnel->dev->stats;
int err = -1;
u8 proto;
- int pkt_len;
struct sk_buff *new_skb;
if (dev->type == ARPHRD_ETHER)
@@ -801,23 +800,9 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
}
}
- nf_reset(skb);
- pkt_len = skb->len;
- err = ip6_local_out(skb);
-
- if (net_xmit_eval(err) == 0) {
- struct pcpu_tstats *tstats = this_cpu_ptr(tunnel->dev->tstats);
-
- tstats->tx_bytes += pkt_len;
- tstats->tx_packets++;
- } else {
- stats->tx_errors++;
- stats->tx_aborted_errors++;
- }
-
+ ip6tunnel_xmit(skb, dev);
if (ndst)
ip6_tnl_dst_store(tunnel, ndst);
-
return 0;
tx_err_link_failure:
stats->tx_carrier_errors++;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index fff83cb..bef3fed 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -955,7 +955,6 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
unsigned int max_headroom = sizeof(struct ipv6hdr);
u8 proto;
int err = -1;
- int pkt_len;
if (!fl6->flowi6_mark)
dst = ip6_tnl_dst_check(t);
@@ -1035,19 +1034,7 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
ipv6h->nexthdr = proto;
ipv6h->saddr = fl6->saddr;
ipv6h->daddr = fl6->daddr;
- nf_reset(skb);
- pkt_len = skb->len;
- err = ip6_local_out(skb);
-
- if (net_xmit_eval(err) == 0) {
- struct pcpu_tstats *tstats = this_cpu_ptr(t->dev->tstats);
-
- tstats->tx_bytes += pkt_len;
- tstats->tx_packets++;
- } else {
- stats->tx_errors++;
- stats->tx_aborted_errors++;
- }
+ ip6tunnel_xmit(skb, dev);
if (ndst)
ip6_tnl_dst_store(t, ndst);
return 0;
^ permalink raw reply related
* [PATCH net-next] don't compare skb->network_header with skb->tail
From: Hong Zhiguo @ 2013-03-10 8:42 UTC (permalink / raw)
To: netdev, davem, stephen; +Cc: zhiguo.hong, Hong Zhiguo
in the case of NET_SKBUFF_DATA_USES_OFFSET(on 64-bit arch),
skb->network_header is just offset over skb->head.
Signed-off-by: Hong Zhiguo <honkiko@gmail.com>
---
net/core/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 90cee5b..378e0b9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1705,7 +1705,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
skb_reset_mac_header(skb2);
if (skb_network_header(skb2) < skb2->data ||
- skb2->network_header > skb2->tail) {
+ skb_network_header(skb2) > skb2->tail) {
net_crit_ratelimited("protocol %04x is buggy, dev %s\n",
ntohs(skb2->protocol),
dev->name);
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net-next 2/2] bnx2x: use the default NAPI weight
From: Herbert Xu @ 2013-03-10 8:38 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, eilong, jhs, Tom Herbert
In-Reply-To: <1362606777.15793.198.camel@edumazet-glaptop>
On Wed, Mar 06, 2013 at 01:52:57PM -0800, Eric Dumazet wrote:
>
> - BQL (incurring more TX completion rounds and possibility to
> block/unblock a qdisc)
> - ticket spinlocks, and even with the guard of qdisc busylock
>
> -> we can have a starvation problem.
This only happens in cases where we aren't using multiqueue or
we're using it incorrectly, resulting in TX work from being split
over CPUs.
In that case it's not clear that it is starvation if we keep the
TX processing on one CPU.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [Patch net-next] tunnel: use iptunnel_xmit() again
From: David Miller @ 2013-03-10 7:05 UTC (permalink / raw)
To: eric.dumazet; +Cc: xiyou.wangcong, netdev, pshelar, stephen
In-Reply-To: <1362889787.4051.14.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 10 Mar 2013 05:29:47 +0100
> On Sun, 2013-03-10 at 10:38 +0800, Cong Wang wrote:
>> From: Cong Wang <xiyou.wangcong@gmail.com>
>>
>> With recent patches from Pravin, most tunnels can't use iptunnel_xmit()
>> any more, due to ip_select_ident() and skb->ip_summed. But we can just
>> move these operations out of iptunnel_xmit(), so that tunnels can
>> use it again.
>>
>> This by the way fixes a bug in vxlan (missing nf_reset()) for net-next.
>>
>> Cc: Pravin B Shelar <pshelar@nicira.com>
>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>
>> ---
>
> missing diffstat
>
> otherwise, patch looks good to me
>
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] Rate should be u64 to avoid integer overflow at high speeds (>= ~35Gbit)
From: Bill Fink @ 2013-03-10 5:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Vimal, netdev, shemminger
In-Reply-To: <1362891937.4051.25.camel@edumazet-glaptop>
On Sun, 10 Mar 2013, Eric Dumazet wrote:
> On Sat, 2013-03-09 at 20:53 -0800, Vimal wrote:
> > Ok, do you have suggestions on how to do this? Maybe a better way to
> > do this would be to introduce an additional "multipler" option for
> > rates, which is set to 1 as default, so actual rate can be computed as
> > multipler * rate supplied.
>
> How an old program, in binary form, will automatically knows it has to
> change its behavior to use an inexistent field ?
>
> I can use an old distro, and update kernel to upstream kernel, it must
> continue to work.
I don't see the problem. An old program would not know about
the new multiplier, would thus get the default multiplier of 1,
and get the same behavior as always, with the same limitation
of ~34 Gbps. But someone with a newer tc/kernel could for example
specify a multiplier of 10, which would then support rates up to
about 340 Gbps. It sounds like a reasonable approach to me.
-Bill
^ permalink raw reply
* Re: [PATCH] Rate should be u64 to avoid integer overflow at high speeds (>= ~35Gbit)
From: Eric Dumazet @ 2013-03-10 5:54 UTC (permalink / raw)
To: Bill Fink; +Cc: Vimal, netdev, shemminger
In-Reply-To: <20130310004904.de508bfa.billfink@mindspring.com>
On Sun, 2013-03-10 at 00:49 -0500, Bill Fink wrote:
> I don't see the problem. An old program would not know about
> the new multiplier, would thus get the default multiplier of 1,
> and get the same behavior as always, with the same limitation
> of ~34 Gbps. But someone with a newer tc/kernel could for example
> specify a multiplier of 10, which would then support rates up to
> about 340 Gbps. It sounds like a reasonable approach to me.
Hopefully, some of us see the problem here and are able to reject
patches before breaking user land.
^ permalink raw reply
* Re: [PATCH] Rate should be u64 to avoid integer overflow at high speeds (>= ~35Gbit)
From: Eric Dumazet @ 2013-03-10 5:05 UTC (permalink / raw)
To: Vimal; +Cc: netdev, shemminger
In-Reply-To: <CAK3Ji11Mij3Y_sGC6nLS01rxfccZo0SPJmQy-LAmskz1OEdmkQ@mail.gmail.com>
On Sat, 2013-03-09 at 20:53 -0800, Vimal wrote:
> Ok, do you have suggestions on how to do this? Maybe a better way to
> do this would be to introduce an additional "multipler" option for
> rates, which is set to 1 as default, so actual rate can be computed as
> multipler * rate supplied.
How an old program, in binary form, will automatically knows it has to
change its behavior to use an inexistent field ?
I can use an old distro, and update kernel to upstream kernel, it must
continue to work.
^ permalink raw reply
* Re: [PATCH] Rate should be u64 to avoid integer overflow at high speeds (>= ~35Gbit)
From: Vimal @ 2013-03-10 4:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, shemminger
In-Reply-To: <1362888229.4051.2.camel@edumazet-glaptop>
Ok, do you have suggestions on how to do this? Maybe a better way to
do this would be to introduce an additional "multipler" option for
rates, which is set to 1 as default, so actual rate can be computed as
multipler * rate supplied.
On 9 March 2013 20:03, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sat, 2013-03-09 at 19:20 -0800, Vimalkumar wrote:
>> Since rate values are passed around between kernel and tc
>> in bytes/sec, 2**32 bytes/sec is around 34Gb/s. Beyond that
>> rate, htb, tbf, hfsc, etc. can never be configured correctly.
>>
>> Signed-off-by: Vimalkumar <j.vimal@gmail.com>
>> ---
>> include/linux/gen_stats.h | 2 +-
>> include/linux/pkt_sched.h | 10 +++++-----
>> misc/ifstat.c | 4 ++--
>> tc/m_police.c | 2 +-
>> tc/q_cbq.c | 2 +-
>> tc/q_choke.c | 2 +-
>> tc/q_gred.c | 2 +-
>> tc/q_hfsc.c | 6 ++++--
>> tc/q_red.c | 2 +-
>> tc/tc_cbq.c | 4 ++--
>> tc/tc_cbq.h | 4 ++--
>> tc/tc_core.c | 4 ++--
>> tc/tc_core.h | 4 ++--
>> tc/tc_util.c | 6 +++---
>> tc/tc_util.h | 6 +++---
>> 15 files changed, 31 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/linux/gen_stats.h b/include/linux/gen_stats.h
>> index 552c8a0..5ca6015 100644
>> --- a/include/linux/gen_stats.h
>> +++ b/include/linux/gen_stats.h
>> @@ -33,7 +33,7 @@ struct gnet_stats_basic_packed {
>> * @pps: current packet rate
>> */
>> struct gnet_stats_rate_est {
>> - __u32 bps;
>> + __u64 bps;
>> __u32 pps;
>> };
>>
>> diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
>> index 32aef0a..d6bc658 100644
>> --- a/include/linux/pkt_sched.h
>> +++ b/include/linux/pkt_sched.h
>> @@ -35,7 +35,7 @@ struct tc_stats {
>> __u32 drops; /* Packets dropped because of lack of resources */
>> __u32 overlimits; /* Number of throttle events when this
>> * flow goes out of allocated bandwidth */
>> - __u32 bps; /* Current flow byte rate */
>> + __u64 bps; /* Current flow byte rate */
>> __u32 pps; /* Current flow packet rate */
>> __u32 qlen;
>> __u32 backlog;
>> @@ -79,7 +79,7 @@ struct tc_ratespec {
>> unsigned short overhead;
>> short cell_align;
>> unsigned short mpu;
>> - __u32 rate;
>> + __u64 rate;
>> };
>>
>
> You cannot do that without breaking user land tools.
>
> Not only tc, but all user applications as well.
>
> We need to support compatibility, before considering adding such
> changes.
>
>
>
--
Vimal
^ permalink raw reply
* Re: [PATCH RFC] unix: account skb memory to receiving socket's sk_rmem_alloc on sending
From: Hannes Frederic Sowa @ 2013-03-10 4:40 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, yannick, xiyou.wangcong, davem
In-Reply-To: <1362889861.4051.15.camel@edumazet-glaptop>
On Sun, Mar 10, 2013 at 05:31:01AM +0100, Eric Dumazet wrote:
> On Sat, 2013-03-09 at 22:43 +0100, Hannes Frederic Sowa wrote:
>
> > I had this patch left from the last net-next submission timeframe. In
> > the meantime I did some updates I would love to have a review on. It
> > e.g. includes poll handling now.
> >
> > This patch lacks documentation updates for max_dgram_qlen. I'll update
> > the documentation on the next submission.
> >
> > Btw, iproute from current git has the ability to report socket memory for
> > unix domain sockets, too. So these changes should be better observable.
>
> I am busy this week attending Netfilter Workshop in Copenhagen.
>
> Do you have a user test program ?
I used a couple of perl scripts. I'll bring them in shape and will post them
here, hopefully tomorrow.
Have great fun in Copenhagen!
^ permalink raw reply
* Re: [PATCH RFC] unix: account skb memory to receiving socket's sk_rmem_alloc on sending
From: Eric Dumazet @ 2013-03-10 4:31 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: netdev, yannick, xiyou.wangcong, davem
In-Reply-To: <20130309214333.GI28531@order.stressinduktion.org>
On Sat, 2013-03-09 at 22:43 +0100, Hannes Frederic Sowa wrote:
> I had this patch left from the last net-next submission timeframe. In
> the meantime I did some updates I would love to have a review on. It
> e.g. includes poll handling now.
>
> This patch lacks documentation updates for max_dgram_qlen. I'll update
> the documentation on the next submission.
>
> Btw, iproute from current git has the ability to report socket memory for
> unix domain sockets, too. So these changes should be better observable.
I am busy this week attending Netfilter Workshop in Copenhagen.
Do you have a user test program ?
^ permalink raw reply
* Re: [Patch net-next] tunnel: use iptunnel_xmit() again
From: Eric Dumazet @ 2013-03-10 4:29 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, Pravin B Shelar, Stephen Hemminger, David S. Miller
In-Reply-To: <1362883119-4099-1-git-send-email-xiyou.wangcong@gmail.com>
On Sun, 2013-03-10 at 10:38 +0800, Cong Wang wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
>
> With recent patches from Pravin, most tunnels can't use iptunnel_xmit()
> any more, due to ip_select_ident() and skb->ip_summed. But we can just
> move these operations out of iptunnel_xmit(), so that tunnels can
> use it again.
>
> This by the way fixes a bug in vxlan (missing nf_reset()) for net-next.
>
> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> ---
missing diffstat
otherwise, patch looks good to me
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH] print_rate: use knowledge in rate_suffix table for human readable rates.
From: Eric Dumazet @ 2013-03-10 4:08 UTC (permalink / raw)
To: Vimalkumar; +Cc: netdev, shemminger
In-Reply-To: <1362885998-14292-1-git-send-email-j.vimal@gmail.com>
On Sat, 2013-03-09 at 19:26 -0800, Vimalkumar wrote:
> Signed-off-by: Vimalkumar <j.vimal@gmail.com>
> ---
> tc/tc_util.c | 26 ++++++++++++--------------
> 1 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/tc/tc_util.c b/tc/tc_util.c
> index 0939536..6e68d87 100644
> --- a/tc/tc_util.c
> +++ b/tc/tc_util.c
> @@ -139,7 +139,8 @@ static const struct rate_suffix {
> { "GBps", 8000000000. },
> { "TiBps", 8.*1024.*1024.*1024.*1024. },
> { "TBps", 8000000000000. },
> - { NULL }
> + { NULL },
> + { NULL },
> };
>
You'll have to explicitly add in the title a way to say its a patch for
iproute2.
[PATCH iproute2] tc: use knowledge in rate_suffix table for human readable rates.
To avoid the confusion, as netdev is more likely to review kernel patches.
^ permalink raw reply
* Re: [PATCH] sch: Rate (in bits/sec) should be u64 so it doesn't overflow at 40Gbit.
From: Eric Dumazet @ 2013-03-10 4:06 UTC (permalink / raw)
To: Vimalkumar; +Cc: netdev, shemminger, davem, jiri, jhs
In-Reply-To: <1362885896-14123-1-git-send-email-j.vimal@gmail.com>
On Sat, 2013-03-09 at 19:24 -0800, Vimalkumar wrote:
> Signed-off-by: Vimalkumar <j.vimal@gmail.com>
> ---
> include/net/sch_generic.h | 4 ++--
> include/uapi/linux/gen_stats.h | 2 +-
> include/uapi/linux/pkt_sched.h | 10 +++++-----
> net/sched/sch_generic.c | 2 +-
> net/sched/sch_hfsc.c | 8 ++++----
> 5 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index f10818f..c9098c8 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -690,9 +690,9 @@ static inline u64 psched_l2t_ns(const struct psched_ratecfg *r,
> return ((u64)len * r->mult) >> r->shift;
> }
>
> -extern void psched_ratecfg_precompute(struct psched_ratecfg *r, u32 rate);
> +extern void psched_ratecfg_precompute(struct psched_ratecfg *r, u64 rate);
>
> -static inline u32 psched_ratecfg_getrate(const struct psched_ratecfg *r)
> +static inline u64 psched_ratecfg_getrate(const struct psched_ratecfg *r)
> {
> return r->rate_bps >> 3;
> }
> diff --git a/include/uapi/linux/gen_stats.h b/include/uapi/linux/gen_stats.h
> index 552c8a0..5ca6015 100644
> --- a/include/uapi/linux/gen_stats.h
> +++ b/include/uapi/linux/gen_stats.h
> @@ -33,7 +33,7 @@ struct gnet_stats_basic_packed {
> * @pps: current packet rate
> */
> struct gnet_stats_rate_est {
> - __u32 bps;
> + __u64 bps;
> __u32 pps;
> };
>
> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
> index 32aef0a..d6bc658 100644
> --- a/include/uapi/linux/pkt_sched.h
> +++ b/include/uapi/linux/pkt_sched.h
> @@ -35,7 +35,7 @@ struct tc_stats {
> __u32 drops; /* Packets dropped because of lack of resources */
> __u32 overlimits; /* Number of throttle events when this
> * flow goes out of allocated bandwidth */
> - __u32 bps; /* Current flow byte rate */
> + __u64 bps; /* Current flow byte rate */
> __u32 pps; /* Current flow packet rate */
> __u32 qlen;
> __u32 backlog;
> @@ -79,7 +79,7 @@ struct tc_ratespec {
> unsigned short overhead;
> short cell_align;
> unsigned short mpu;
> - __u32 rate;
> + __u64 rate;
> };
>
If this was so easy, we would have done that 10 years ago.
This change breaks userland, and thats absolutely not doable.
^ permalink raw reply
* Re: [PATCH] Rate should be u64 to avoid integer overflow at high speeds (>= ~35Gbit)
From: Eric Dumazet @ 2013-03-10 4:03 UTC (permalink / raw)
To: Vimalkumar; +Cc: netdev, shemminger
In-Reply-To: <1362885604-14006-1-git-send-email-j.vimal@gmail.com>
On Sat, 2013-03-09 at 19:20 -0800, Vimalkumar wrote:
> Since rate values are passed around between kernel and tc
> in bytes/sec, 2**32 bytes/sec is around 34Gb/s. Beyond that
> rate, htb, tbf, hfsc, etc. can never be configured correctly.
>
> Signed-off-by: Vimalkumar <j.vimal@gmail.com>
> ---
> include/linux/gen_stats.h | 2 +-
> include/linux/pkt_sched.h | 10 +++++-----
> misc/ifstat.c | 4 ++--
> tc/m_police.c | 2 +-
> tc/q_cbq.c | 2 +-
> tc/q_choke.c | 2 +-
> tc/q_gred.c | 2 +-
> tc/q_hfsc.c | 6 ++++--
> tc/q_red.c | 2 +-
> tc/tc_cbq.c | 4 ++--
> tc/tc_cbq.h | 4 ++--
> tc/tc_core.c | 4 ++--
> tc/tc_core.h | 4 ++--
> tc/tc_util.c | 6 +++---
> tc/tc_util.h | 6 +++---
> 15 files changed, 31 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/gen_stats.h b/include/linux/gen_stats.h
> index 552c8a0..5ca6015 100644
> --- a/include/linux/gen_stats.h
> +++ b/include/linux/gen_stats.h
> @@ -33,7 +33,7 @@ struct gnet_stats_basic_packed {
> * @pps: current packet rate
> */
> struct gnet_stats_rate_est {
> - __u32 bps;
> + __u64 bps;
> __u32 pps;
> };
>
> diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
> index 32aef0a..d6bc658 100644
> --- a/include/linux/pkt_sched.h
> +++ b/include/linux/pkt_sched.h
> @@ -35,7 +35,7 @@ struct tc_stats {
> __u32 drops; /* Packets dropped because of lack of resources */
> __u32 overlimits; /* Number of throttle events when this
> * flow goes out of allocated bandwidth */
> - __u32 bps; /* Current flow byte rate */
> + __u64 bps; /* Current flow byte rate */
> __u32 pps; /* Current flow packet rate */
> __u32 qlen;
> __u32 backlog;
> @@ -79,7 +79,7 @@ struct tc_ratespec {
> unsigned short overhead;
> short cell_align;
> unsigned short mpu;
> - __u32 rate;
> + __u64 rate;
> };
>
You cannot do that without breaking user land tools.
Not only tc, but all user applications as well.
We need to support compatibility, before considering adding such
changes.
^ permalink raw reply
* [PATCH] print_rate: use knowledge in rate_suffix table for human readable rates.
From: Vimalkumar @ 2013-03-10 3:26 UTC (permalink / raw)
To: netdev, shemminger, eric.dumazet; +Cc: Vimalkumar
Signed-off-by: Vimalkumar <j.vimal@gmail.com>
---
tc/tc_util.c | 26 ++++++++++++--------------
1 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/tc/tc_util.c b/tc/tc_util.c
index 0939536..6e68d87 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -139,7 +139,8 @@ static const struct rate_suffix {
{ "GBps", 8000000000. },
{ "TiBps", 8.*1024.*1024.*1024.*1024. },
{ "TBps", 8000000000000. },
- { NULL }
+ { NULL },
+ { NULL },
};
@@ -171,21 +172,18 @@ void print_rate(char *buf, int len, __u64 rate)
{
double tmp = (double)rate*8;
extern int use_iec;
+ int start = 0;
+ const struct rate_suffix *s;
if (use_iec) {
- if (tmp >= 1000.0*1024.0*1024.0)
- snprintf(buf, len, "%.0fMibit", tmp/(1024.0*1024.0));
- else if (tmp >= 1000.0*1024)
- snprintf(buf, len, "%.0fKibit", tmp/1024);
- else
- snprintf(buf, len, "%.0fbit", tmp);
- } else {
- if (tmp >= 1000.0*1000000.0)
- snprintf(buf, len, "%.0fMbit", tmp/1000000.0);
- else if (tmp >= 1000.0 * 1000.0)
- snprintf(buf, len, "%.0fKbit", tmp/1000.0);
- else
- snprintf(buf, len, "%.0fbit", tmp);
+ start = 1;
+ }
+
+ snprintf(buf, len, "%.0fbit", tmp);
+ for (s = &suffixes[start]; s->name; s += 2) {
+ if (tmp >= s->scale) {
+ snprintf(buf, len, "%.3f%s", tmp / s->scale, s->name);
+ }
}
}
--
1.7.5.3
^ permalink raw reply related
* [PATCH] sch: Rate (in bits/sec) should be u64 so it doesn't overflow at 40Gbit.
From: Vimalkumar @ 2013-03-10 3:24 UTC (permalink / raw)
To: netdev, eric.dumazet, shemminger, davem, jiri, jhs; +Cc: Vimalkumar
Signed-off-by: Vimalkumar <j.vimal@gmail.com>
---
include/net/sch_generic.h | 4 ++--
include/uapi/linux/gen_stats.h | 2 +-
include/uapi/linux/pkt_sched.h | 10 +++++-----
net/sched/sch_generic.c | 2 +-
net/sched/sch_hfsc.c | 8 ++++----
5 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f10818f..c9098c8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -690,9 +690,9 @@ static inline u64 psched_l2t_ns(const struct psched_ratecfg *r,
return ((u64)len * r->mult) >> r->shift;
}
-extern void psched_ratecfg_precompute(struct psched_ratecfg *r, u32 rate);
+extern void psched_ratecfg_precompute(struct psched_ratecfg *r, u64 rate);
-static inline u32 psched_ratecfg_getrate(const struct psched_ratecfg *r)
+static inline u64 psched_ratecfg_getrate(const struct psched_ratecfg *r)
{
return r->rate_bps >> 3;
}
diff --git a/include/uapi/linux/gen_stats.h b/include/uapi/linux/gen_stats.h
index 552c8a0..5ca6015 100644
--- a/include/uapi/linux/gen_stats.h
+++ b/include/uapi/linux/gen_stats.h
@@ -33,7 +33,7 @@ struct gnet_stats_basic_packed {
* @pps: current packet rate
*/
struct gnet_stats_rate_est {
- __u32 bps;
+ __u64 bps;
__u32 pps;
};
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 32aef0a..d6bc658 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -35,7 +35,7 @@ struct tc_stats {
__u32 drops; /* Packets dropped because of lack of resources */
__u32 overlimits; /* Number of throttle events when this
* flow goes out of allocated bandwidth */
- __u32 bps; /* Current flow byte rate */
+ __u64 bps; /* Current flow byte rate */
__u32 pps; /* Current flow packet rate */
__u32 qlen;
__u32 backlog;
@@ -79,7 +79,7 @@ struct tc_ratespec {
unsigned short overhead;
short cell_align;
unsigned short mpu;
- __u32 rate;
+ __u64 rate;
};
#define TC_RTAB_SIZE 1024
@@ -368,9 +368,9 @@ struct tc_hfsc_qopt {
};
struct tc_service_curve {
- __u32 m1; /* slope of the first segment in bps */
+ __u64 m1; /* slope of the first segment in bps */
__u32 d; /* x-projection of the first segment in us */
- __u32 m2; /* slope of the second segment in bps */
+ __u64 m2; /* slope of the second segment in bps */
};
struct tc_hfsc_stats {
@@ -541,7 +541,7 @@ struct tc_netem_corrupt {
};
struct tc_netem_rate {
- __u32 rate; /* byte/s */
+ __u64 rate; /* byte/s */
__s32 packet_overhead;
__u32 cell_size;
__s32 cell_overhead;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ffad481..37a178c 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -898,7 +898,7 @@ void dev_shutdown(struct net_device *dev)
WARN_ON(timer_pending(&dev->watchdog_timer));
}
-void psched_ratecfg_precompute(struct psched_ratecfg *r, u32 rate)
+void psched_ratecfg_precompute(struct psched_ratecfg *r, u64 rate)
{
u64 factor;
u64 mult;
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 9facea0..f06b447 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -419,7 +419,7 @@ seg_y2x(u64 y, u64 ism)
/* Convert m (bps) into sm (bytes/psched us) */
static u64
-m2sm(u32 m)
+m2sm(u64 m)
{
u64 sm;
@@ -431,7 +431,7 @@ m2sm(u32 m)
/* convert m (bps) into ism (psched us/byte) */
static u64
-m2ism(u32 m)
+m2ism(u64 m)
{
u64 ism;
@@ -458,13 +458,13 @@ d2dx(u32 d)
}
/* convert sm (bytes/psched us) into m (bps) */
-static u32
+static u64
sm2m(u64 sm)
{
u64 m;
m = (sm * PSCHED_TICKS_PER_SEC) >> SM_SHIFT;
- return (u32)m;
+ return m;
}
/* convert dx (psched us) into d (us) */
--
1.7.5.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox