* Re: [PATCH net] r8169: Enable MSI-X on RTL8106e
From: Jian-Hong Pan @ 2018-10-02 5:57 UTC (permalink / raw)
To: David S. Miller
Cc: Heiner Kallweit, Realtek linux nic maintainers, Linux Netdev List,
Linux Kernel, Kai-Heng Feng, Linux Upstreaming Team
In-Reply-To: <20181001.225140.1939583158282424865.davem@davemloft.net>
David Miller <davem@davemloft.net> 於 2018年10月2日 週二 下午1:51寫道：
>
> From: Jian-Hong Pan <jian-hong@endlessm.com>
> Date: Thu, 27 Sep 2018 12:09:48 +0800
>
> > However, there is a commit which resolves the drivers getting nothing in
> > PCI BAR=4 after system resumes. It is 04cb3ae895d7 "PCI: Reprogram
> > bridge prefetch registers on resume" by Daniel Drake.
>
> I don't see this upstream yet.
It is in linux-next repository:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=04cb3ae895d7efdc60f0fe17182b200a3da20f09
Regrads,
Jian-Hong Pan
^ permalink raw reply
* Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb
From: Andrew Lunn @ 2018-10-02 12:43 UTC (permalink / raw)
To: Antoine Tenart
Cc: Florian Fainelli, davem, netdev, linux-kernel, thomas.petazzoni,
alexandre.belloni, quentin.schulz, allan.nielsen
In-Reply-To: <20181002065937.GB3368@kwain>
> The question could be "do we need len += ETH_FCS_LEN" to account for the
> FCS when NETIF_F_RXFCS is used", but I looked at other drivers and it
> seemed to me the FCS is not accounted in the stats. Should it be?
Hi Antoine
There does not appear to be a good answer to that. I submitted a patch
to a driver i'm using to not count it, so that the stats counters we
consistent with another driver. The patch was rejected.
I think the best you can do is flip a coin, and then document it using
a comment about if it is/is not included.
Andrew
^ permalink raw reply
* Re: [PATCH bpf] bpf: don't accept cgroup local storage with zero value size
From: Daniel Borkmann @ 2018-10-02 12:43 UTC (permalink / raw)
To: Roman Gushchin, netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Kernel Team, Alexei Starovoitov
In-Reply-To: <20181002024003.25635-1-guro@fb.com>
On 10/02/2018 04:41 AM, Roman Gushchin wrote:
> Explicitly forbid creating cgroup local storage maps with zero value
> size, as it makes no sense and might even cause a panic.
>
> Reported-by: syzbot+18628320d3b14a5c459c@syzkaller.appspotmail.com
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
Applied to bpf, thanks Roman!
^ permalink raw reply
* Re: [PATCH 1/4] usbnet: smsc95xx: add kconfig for turbo mode
From: Andrew Lunn @ 2018-10-02 12:46 UTC (permalink / raw)
To: Ben Dooks; +Cc: netdev, oneukum, davem, linux-usb, linux-kernel, linux-kernel
In-Reply-To: <20181002092645.1115-2-ben.dooks@codethink.co.uk>
On Tue, Oct 02, 2018 at 10:26:42AM +0100, Ben Dooks wrote:
> Add a configuration option for the default state of turbo mode
> on the smsc95xx networking driver. Some systems it is better
> to default this to off as it causes significant increases in
> soft-irq load.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> drivers/net/usb/Kconfig | 9 +++++++++
> drivers/net/usb/smsc95xx.c | 2 +-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 418b0904cecb..a32f1a446ce9 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -351,6 +351,15 @@ config USB_NET_SMSC95XX
> This option adds support for SMSC LAN95XX based USB 2.0
> 10/100 Ethernet adapters.
>
> +config USB_NET_SMSC95XX_TURBO
> + bool "Use turbo receive mode by default"
> + depends on USB_NET_SMSC95XX
> + default y
> + help
> + This options sets the default turbo mode settings for the
> + driver's receive path. These can also be altered by the
> + turbo_mode module parameter.
> +
Hi Ben
Is it worth adding a comment here why you would want to turn it off?
To reduce soft-irq load?
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net-next 0/5] net: systemport: Turn on offloads by
From: David Miller @ 2018-10-02 6:12 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev
In-Reply-To: <20180927223614.7116-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 27 Sep 2018 15:36:09 -0700
> Up until now, we had added all the code necessary to turn on RX/TX
> checksum offloads at runtime, but there is no reason why they have to be
> disabled by default given that this gives a slight performance
> improvement.
Series applied.
^ permalink raw reply
* Re: [PATCH v2 net-next] net: phy: improve handling delayed work
From: David Miller @ 2018-10-02 6:14 UTC (permalink / raw)
To: hkallweit1; +Cc: f.fainelli, andrew, netdev
In-Reply-To: <a35342df-36ad-9364-37f5-39176809bab0@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Fri, 28 Sep 2018 08:51:09 +0200
> Using mod_delayed_work() allows to simplify handling delayed work and
> removes the need for the sync parameter in phy_trigger_machine().
> Also introduce a helper phy_queue_state_machine() to encapsulate the
> low-level delayed work calls. No functional change intended.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - removed inline annotation from phy_queue_state_machine()
Applied.
^ permalink raw reply
* Re: [PATCH net-next] tcp/fq: move back to CLOCK_MONOTONIC
From: David Miller @ 2018-10-02 6:19 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, leonard.crestez
In-Reply-To: <20180928172844.182542-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 28 Sep 2018 10:28:44 -0700
> In the recent TCP/EDT patch series, I switched TCP and sch_fq
> clocks from MONOTONIC to TAI, in order to meet the choice done
> earlier for sch_etf packet scheduler.
>
> But sure enough, this broke some setups were the TAI clock
> jumps forward (by almost 50 year...), as reported
> by Leonard Crestez.
>
> If we want to converge later, we'll probably need to add
> an skb field to differentiate the clock bases, or a socket option.
>
> In the meantime, an UDP application will need to use CLOCK_MONOTONIC
> base for its SCM_TXTIME timestamps if using fq packet scheduler.
>
> Fixes: 72b0094f9182 ("tcp: switch tcp_clock_ns() to CLOCK_TAI base")
> Fixes: 142537e41923 ("net_sched: sch_fq: switch to CLOCK_TAI")
> Fixes: fd2bca2aa789 ("tcp: switch internal pacing timer to CLOCK_TAI")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] rtnetlink: Fail dump if target netnsid is invalid
From: David Miller @ 2018-10-02 6:22 UTC (permalink / raw)
To: dsahern; +Cc: netdev, jbenc, dsahern
In-Reply-To: <20180928192841.20410-1-dsahern@kernel.org>
From: David Ahern <dsahern@kernel.org>
Date: Fri, 28 Sep 2018 12:28:41 -0700
> From: David Ahern <dsahern@gmail.com>
>
> Link dumps can return results from a target namespace. If the namespace id
> is invalid, then the dump request should fail if get_target_net fails
> rather than continuing with a dump of the current namespace.
>
> Fixes: 79e1ad148c844 ("rtnetlink: use netnsid to query interface")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* [PATCH net-next] tcp: do not release socket ownership in tcp_close()
From: Eric Dumazet @ 2018-10-02 6:24 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
syzkaller was able to hit the WARN_ON(sock_owned_by_user(sk));
in tcp_close()
While a socket is being closed, it is very possible other
threads find it in rtnetlink dump.
tcp_get_info() will acquire the socket lock for a short amount
of time (slow = lock_sock_fast(sk)/unlock_sock_fast(sk, slow);),
enough to trigger the warning.
Fixes: 67db3e4bfbc9 ("tcp: no longer hold ehash lock while calling tcp_get_info()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
include/net/sock.h | 1 +
net/core/sock.c | 2 +-
net/ipv4/tcp.c | 11 +++--------
3 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 38cae35f6e16056c862b29dee8ceb46fe16fff7b..751549ac0a849144ab0382203ee5c877374523e2 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1492,6 +1492,7 @@ static inline void lock_sock(struct sock *sk)
lock_sock_nested(sk, 0);
}
+void __release_sock(struct sock *sk);
void release_sock(struct sock *sk);
/* BH context may only use the following locking interface. */
diff --git a/net/core/sock.c b/net/core/sock.c
index 8537b6ca72c5013a75c70978c079646f6278aabf..7e8796a6a0892efbb7dfce67d12b8062b2d5daa9 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2317,7 +2317,7 @@ static void __lock_sock(struct sock *sk)
finish_wait(&sk->sk_lock.wq, &wait);
}
-static void __release_sock(struct sock *sk)
+void __release_sock(struct sock *sk)
__releases(&sk->sk_lock.slock)
__acquires(&sk->sk_lock.slock)
{
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 2827fa5643bde84633b174914bbb13a8273e9d32..43ef83b2330e6238a55c9843580a585d87708e0c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2416,16 +2416,10 @@ void tcp_close(struct sock *sk, long timeout)
sock_hold(sk);
sock_orphan(sk);
- /* It is the last release_sock in its life. It will remove backlog. */
- release_sock(sk);
-
-
- /* Now socket is owned by kernel and we acquire BH lock
- * to finish close. No need to check for user refs.
- */
local_bh_disable();
bh_lock_sock(sk);
- WARN_ON(sock_owned_by_user(sk));
+ /* remove backlog if any, without releasing ownership. */
+ __release_sock(sk);
percpu_counter_inc(sk->sk_prot->orphan_count);
@@ -2494,6 +2488,7 @@ void tcp_close(struct sock *sk, long timeout)
out:
bh_unlock_sock(sk);
local_bh_enable();
+ release_sock(sk);
sock_put(sk);
}
EXPORT_SYMBOL(tcp_close);
--
2.19.0.605.g01d371f741-goog
^ permalink raw reply related
* Re: bond: take rcu lock in netpoll_send_skb_on_dev
From: David Miller @ 2018-10-02 6:26 UTC (permalink / raw)
To: davej; +Cc: netdev, xiyou.wangcong
In-Reply-To: <20180928202608.uycdlytob75iphfu@codemonkey.org.uk>
From: Dave Jones <davej@codemonkey.org.uk>
Date: Fri, 28 Sep 2018 16:26:08 -0400
> The bonding driver lacks the rcu lock when it calls down into
> netdev_lower_get_next_private_rcu from bond_poll_controller, which
> results in a trace like:
...
> We're also doing rcu dereferences a layer up in netpoll_send_skb_on_dev
> before we call down into netpoll_poll_dev, so just take the lock there.
>
> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Dave Jones <davej@codemonkey.org.uk>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net 0/3] tun: address two syzbot reports
From: David Miller @ 2018-10-02 6:28 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet
In-Reply-To: <20180928215149.22092-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 28 Sep 2018 14:51:46 -0700
> Small changes addressing races discovered by syzbot.
>
> First patch is a cleanup.
> Second patch moves a mutex init sooner.
> Third patch makes sure each tfile gets its own napi enable flags.
Series applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net v2] r8169: fix network stalls due to missing bit TXCFG_AUTO_FIFO
From: David Miller @ 2018-10-02 6:29 UTC (permalink / raw)
To: hkallweit1; +Cc: nic_swsd, netdev, tatkinson, admin, odi
In-Reply-To: <1559697e-9dce-e162-21b1-763b1286cd1a@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Fri, 28 Sep 2018 23:51:54 +0200
> Some of the chip-specific hw_start functions set bit TXCFG_AUTO_FIFO
> in register TxConfig. The original patch changed the order of some
> calls resulting in these changes being overwritten by
> rtl_set_tx_config_registers() in rtl_hw_start(). This eventually
> resulted in network stalls especially under high load.
>
> Analyzing the chip-specific hw_start functions all chip version from
> 34, with the exception of version 39, need this bit set.
> This patch moves setting this bit to rtl_set_tx_config_registers().
>
> Fixes: 4fd48c4ac0a0 ("r8169: move common initializations to tp->hw_start")
> Reported-by: Ortwin Glück <odi@odi.ch>
> Reported-by: David Arendt <admin@prnet.org>
> Root-caused-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Tested-by: Tony Atkinson <tatkinson@linux.com>
> Tested-by: David Arendt <admin@prnet.org>
> Tested-by: Ortwin Glück <odi@odi.ch>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> Hint for applying this change to stable:
> It may collide with 05212ba8132b ("r8169: set RxConfig after tx/rx is
> enabled for RTL8169sb/8110sb devices") which renamed
> rtl_set_rx_tx_config_registers() to rtl_set_tx_config_registers().
> ---
> v2:
> - added Maciej as root-caused-by
Applied and queued up for -stable, thanks for the backporting hints.
^ permalink raw reply
* Re: [PATCH net-next 0/3] ibmvnic: Implement driver-defined queue limits
From: David Miller @ 2018-10-02 6:31 UTC (permalink / raw)
To: tlfalcon; +Cc: netdev, nfont
In-Reply-To: <1538177906-17068-1-git-send-email-tlfalcon@linux.ibm.com>
From: Thomas Falcon <tlfalcon@linux.ibm.com>
Date: Fri, 28 Sep 2018 18:38:23 -0500
> In this patch series, update the ibmvnic driver to use driver-defined
> queue limits instead of limits imposed by the Virtual I/O server
> management partition. For some deviced, initial max queue size and
> amount limits, despite their definition, can actually be exceeded if
> the client driver requests it. With this in mind, define a private
> ethtool flag that toggles the use of driver-defined limits. These
> limits are currently more than what supported hardware will likely
> allow, so the driver will attempt to get as close as possible to
> the user request but may not fully succeed.
Series applied.
^ permalink raw reply
* RE: [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word
From: David Laight @ 2018-10-02 13:19 UTC (permalink / raw)
To: 'Ben Dooks', netdev@vger.kernel.org
Cc: oneukum@suse.com, davem@davemloft.net, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kernel@lists.codethink.co.uk
In-Reply-To: <20181002092645.1115-3-ben.dooks@codethink.co.uk>
From: Ben Dooks
> Sent: 02 October 2018 10:27
>
> The tegra driver requires alignment of the buffer, so try and
> make this better by pushing the buffer start back to an word
> aligned address. At the worst this makes memcpy() easier as
> it is word aligned, at best it makes sure the usb can directly
> map the buffer.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> [todo - make this configurable]
> ---
> drivers/net/usb/Kconfig | 12 ++++++++++++
> drivers/net/usb/smsc95xx.c | 22 ++++++++++++++++++++--
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
...
> +static bool align_tx = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN);
> +module_param(align_tx, bool, 0644);
> +MODULE_PARM_DESC(align_tx, "Align TX buffers to word boundaries");
DM doesn't like module parameters.
> static bool turbo_mode = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TURBO);
> module_param(turbo_mode, bool, 0644);
> MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
> @@ -2005,10 +2009,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> bool csum = skb->ip_summed == CHECKSUM_PARTIAL;
> int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM : SMSC95XX_TX_OVERHEAD;
> u32 tx_cmd_a, tx_cmd_b;
> + u32 data_len;
> + uintptr_t align = 0;
>
> /* We do not advertise SG, so skbs should be already linearized */
> BUG_ON(skb_shinfo(skb)->nr_frags);
>
> + if (IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN) && align_tx) {
> + align = (uintptr_t)skb->data & 3;
> + if (align)
> + overhead += 4 - align;
Better to calculate the pad size once:
align = (-(long)skb->data) & 3;
should do it - and you can unconditionally add it in.
> + }
> +
> /* Make writable and expand header space by overhead if required */
> if (skb_cow_head(skb, overhead)) {
> /* Must deallocate here as returning NULL to indicate error
> @@ -2037,16 +2049,22 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> }
> }
>
> + data_len = skb->len;
> + if (align)
> + skb_push(skb, 4 - align);
> +
> skb_push(skb, 4);
You don't want to call skb_push() twice.
IIRC really horrid things happen if the data has to be copied.
(Actually what happens to the alignment in that case??)
And there is another skb_push() below....
> - tx_cmd_b = (u32)(skb->len - 4);
> + tx_cmd_b = (u32)(data_len);
You don't need the cast here at all (if it was ever needed).
Actually you don't need the new 'data_len' variable.
Just set tx_cmd_b earlier.
...
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* [PATCH] declance: Fix continuation with the adapter identification message
From: Maciej W. Rozycki @ 2018-10-02 13:23 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel
Fix a commit 4bcc595ccd80 ("printk: reinstate KERN_CONT for printing
continuation lines") regression with the `declance' driver, which caused
the adapter identification message to be split between two lines, e.g.:
declance.c: v0.011 by Linux MIPS DECstation task force
tc6: PMAD-AA
, addr = 08:00:2b:1b:2a:6a, irq = 14
tc6: registered as eth0.
Address that properly, by printing identification with a single call,
making the messages now look like:
declance.c: v0.011 by Linux MIPS DECstation task force
tc6: PMAD-AA, addr = 08:00:2b:1b:2a:6a, irq = 14
tc6: registered as eth0.
Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
Fixes: 4bcc595ccd80 ("printk: reinstate KERN_CONT for printing continuation lines")
---
Hi,
Not requesting a backport as the impact of this issue is purely cosmetic.
Deferring driver review to use `netdev_info', etc. in place of `printk' to
another time. Please apply.
Maciej
---
drivers/net/ethernet/amd/declance.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
linux-net-declance-pr-cont.patch
Index: linux-20180930-4maxp64/drivers/net/ethernet/amd/declance.c
===================================================================
--- linux-20180930-4maxp64.orig/drivers/net/ethernet/amd/declance.c
+++ linux-20180930-4maxp64/drivers/net/ethernet/amd/declance.c
@@ -1031,6 +1031,7 @@ static int dec_lance_probe(struct device
int i, ret;
unsigned long esar_base;
unsigned char *esar;
+ const char *desc;
if (dec_lance_debug && version_printed++ == 0)
printk(version);
@@ -1216,19 +1217,20 @@ static int dec_lance_probe(struct device
*/
switch (type) {
case ASIC_LANCE:
- printk("%s: IOASIC onboard LANCE", name);
+ desc = "IOASIC onboard LANCE";
break;
case PMAD_LANCE:
- printk("%s: PMAD-AA", name);
+ desc = "PMAD-AA";
break;
case PMAX_LANCE:
- printk("%s: PMAX onboard LANCE", name);
+ desc = "PMAX onboard LANCE";
break;
}
for (i = 0; i < 6; i++)
dev->dev_addr[i] = esar[i * 4];
- printk(", addr = %pM, irq = %d\n", dev->dev_addr, dev->irq);
+ printk("%s: %s, addr = %pM, irq = %d\n",
+ name, desc, dev->dev_addr, dev->irq);
dev->netdev_ops = &lance_netdev_ops;
dev->watchdog_timeo = 5*HZ;
^ permalink raw reply
* Re: [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word
From: Ben Dooks @ 2018-10-02 13:35 UTC (permalink / raw)
To: David Laight, netdev@vger.kernel.org
Cc: oneukum@suse.com, davem@davemloft.net, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kernel@lists.codethink.co.uk
In-Reply-To: <59988ed22559410881addfecf58335eb@AcuMS.aculab.com>
On 02/10/18 14:19, David Laight wrote:
> From: Ben Dooks
>> Sent: 02 October 2018 10:27
>>
>> The tegra driver requires alignment of the buffer, so try and
>> make this better by pushing the buffer start back to an word
>> aligned address. At the worst this makes memcpy() easier as
>> it is word aligned, at best it makes sure the usb can directly
>> map the buffer.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> [todo - make this configurable]
>> ---
>> drivers/net/usb/Kconfig | 12 ++++++++++++
>> drivers/net/usb/smsc95xx.c | 22 ++++++++++++++++++++--
>> 2 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> ...
>> +static bool align_tx = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN);
>> +module_param(align_tx, bool, 0644);
>> +MODULE_PARM_DESC(align_tx, "Align TX buffers to word boundaries");
>
> DM doesn't like module parameters.
>
>> static bool turbo_mode = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TURBO);
>> module_param(turbo_mode, bool, 0644);
>> MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
>> @@ -2005,10 +2009,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>> bool csum = skb->ip_summed == CHECKSUM_PARTIAL;
>> int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM : SMSC95XX_TX_OVERHEAD;
>> u32 tx_cmd_a, tx_cmd_b;
>> + u32 data_len;
>> + uintptr_t align = 0;
>>
>> /* We do not advertise SG, so skbs should be already linearized */
>> BUG_ON(skb_shinfo(skb)->nr_frags);
>>
>> + if (IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN) && align_tx) {
>> + align = (uintptr_t)skb->data & 3;
>> + if (align)
>> + overhead += 4 - align;
>
> Better to calculate the pad size once:
> align = (-(long)skb->data) & 3;
> should do it - and you can unconditionally add it in.
>
>> + }
>> +
>> /* Make writable and expand header space by overhead if required */
>> if (skb_cow_head(skb, overhead)) {
>> /* Must deallocate here as returning NULL to indicate error
>> @@ -2037,16 +2049,22 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>> }
>> }
>>
>> + data_len = skb->len;
>> + if (align)
>> + skb_push(skb, 4 - align);
>> +
>> skb_push(skb, 4);
>
> You don't want to call skb_push() twice.
> IIRC really horrid things happen if the data has to be copied.
> (Actually what happens to the alignment in that case??)
> And there is another skb_push() below....
The driver does it /multiple/ times depending on the path used.
Is it wise to try and make a separate patch to skb_push() once
and also move the tx_cmd_a and tx_cmd_b bit to a single point?
>> - tx_cmd_b = (u32)(skb->len - 4);
>> + tx_cmd_b = (u32)(data_len);
>
> You don't need the cast here at all (if it was ever needed).
> Actually you don't need the new 'data_len' variable.
> Just set tx_cmd_b earlier.
>
> ...
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
>
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
^ permalink raw reply
* Re: [PATCH net-next] net: mscc: allow extracting the FCS into the skb
From: Antoine Tenart @ 2018-10-02 6:59 UTC (permalink / raw)
To: Florian Fainelli
Cc: Antoine Tenart, davem, netdev, linux-kernel, thomas.petazzoni,
alexandre.belloni, quentin.schulz, allan.nielsen
In-Reply-To: <25a88126-2397-a1fb-0c0d-06504bb348ca@gmail.com>
Hi FLorian,
On Mon, Oct 01, 2018 at 09:35:45AM -0700, Florian Fainelli wrote:
> On 10/01/2018 02:57 AM, Antoine Tenart wrote:
> >
> > memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
> > diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
> > index 3cdf63e35b53..245452a0f244 100644
> > --- a/drivers/net/ethernet/mscc/ocelot_board.c
> > +++ b/drivers/net/ethernet/mscc/ocelot_board.c
> > @@ -126,11 +126,16 @@ static irqreturn_t ocelot_xtr_irq_handler(int irq, void *arg)
> > len += sz;
> > } while (len < buf_len);
> >
> > - /* Read the FCS and discard it */
> > + /* Read the FCS */
> > sz = ocelot_rx_frame_word(ocelot, grp, false, &val);
> > /* Update the statistics if part of the FCS was read before */
> > len -= ETH_FCS_LEN - sz;
> >
> Don't this needs to be len -= sz;
No, part of the FCS could be read in the loop before. This subtraction
removes those bytes from the stats.
> > + if (unlikely(dev->features & NETIF_F_RXFCS)) {
> > + buf = (u32 *)skb_put(skb, ETH_FCS_LEN);
> > + *buf = val;
>
> and here len -= ETH_FCS_LEN
len doesn't contain the FCS len at this point, no need to remove it
again. The extraction logic is already designed to drop the FCS and to
not count it in the stats, this patch makes use of it depending on
NETIF_F_RXFCS.
> since "len" is later used for accounting how many bytes have been
> received by the network device?
The question could be "do we need len += ETH_FCS_LEN" to account for the
FCS when NETIF_F_RXFCS is used", but I looked at other drivers and it
seemed to me the FCS is not accounted in the stats. Should it be?
> I am bit confused by the use of "buf", but presumably if NETIF_F_RXCFS
> is turned on, the FCS needs to be part of the buffer passed to the
> network stack, so adjusting len accordingly is required anyway.
This is the way we extract the data to the skb, I'm following the same
logic (I could use buf++ as well, as it's done in the loop before).
Thanks!
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters
From: Quentin Schulz @ 2018-10-02 13:51 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, f.fainelli, allan.nielsen, linux-kernel, netdev,
thomas.petazzoni, Raju Lakkaraju, rmk+kernel
In-Reply-To: <20180914132959.GH14865@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 3972 bytes --]
Hi Russel,
Adding you to the discussion as you're the author and commiter of the
patch adding support for all the paged access in the phy core.
On Fri, Sep 14, 2018 at 03:29:59PM +0200, Andrew Lunn wrote:
> > When you change a page, you basically can access only the registers in
> > this page so if there are two functions requesting different pages at
> > the same time or registers of different pages, it won't work well
> > indeed.
> >
> > > phy_read_page() and phy_write_page() will do the needed locking if
> > > this is an issue.
> > >
> >
> > That's awesome! Didn't know it existed. Thanks a ton!
> >
> > Well, that means I should migrate the whole driver to use
> > phy_read/write_paged instead of the phy_read/write that is currently in
> > use.
> >
> > That's impacting performance though as per phy_read/write_paged we read
> > the current page, set the desired page, read/write the register, set the
> > old page back. That's 4 times more operations.
>
> You can use the lower level locking primatives. See m88e1318_set_wol()
> for example.
>
I'm converting the drivers/net/phy/mscc.c driver to make use of the
paged accesses but I'm hitting something confusing to me.
Firstly, just to be sure, I should use paged accesses only for read/write
outside of the standard page, right? I'm guessing that because we need
to be able to use the genphy functions which are using phy_write/read
and not __phy_write/read, thus we assume the mdio lock is not taken
(which is taken by phy_select/restore_page) and those functions
read/write to the standard page.
Secondly, I should refactor the driver to do the following:
oldpage = phy_select_page();
if (oldpage < 0) {
phy_restore_page();
error_path;
}
[...]
/* paged accesses */
__phy_write/read();
[...]
phy_restore_page();
I assume this is the correct way to handle paged accesses. Let me know
if it's not clear enough or wrong. (depending on the function, we could
of course put phy_restore_page in the error_path).
Now, I saw that phy_restore_page takes the phydev, the oldpage and a ret
parameters[1].
The (ret >= 0 && r < 0) condition of [2] seems counterintuitive to me.
ret being the argument passed to the function and r being the return of
__phy_write_page (which is phydev->drv->phy_write_page()).
In my understanding of C best practices, any return value equal to zero
marks a successful call to the function.
That would mean that with:
if (ret >= 0 && r < 0)
ret = r;
If ret is greather than 0, if __phy_write_page is successful (r == 0),
ret will be > 0, which would result in phy_restore_page not returning 0
thus signaling (IMO) an error occured in phy_restore_page.
One example is the following:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage);
If phy_select_page is successful, return phy_restore_page(phydev,
oldpage, oldpage) would return the value of oldpage which can be
different from 0.
This code could (I think) be working with `if (ret >= 0 && r <= 0)` (or
even `if (ret >= 0)`).
Now to have the same behaviour, I need to do:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage > 0 ? 0 : oldpage);
Another example is:
oldpage = phy_select_page(phydev, new_page);
ret = `any function returning a value > 0 in case of success and < 0 in
case of failure`().
return phy_restore_page(phydev, oldpage, ret);
Is there any reason for not wanting to overwrite the ret value when
__phy_write_page is successful in phy_restore_page?
I'd say that it could be more readable without the ternary condition in
the argument of phy_restore_page.
Let me know your thoughts on this.
Thanks,
Quentin
[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L444
[2] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L454
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: bug: 'ethtool -m' reports spurious alarm & warning threshold values for QSFP28 transceivers
From: Eran Ben Elisha @ 2018-10-02 7:10 UTC (permalink / raw)
To: Andrew Lunn
Cc: Eran Ben Elisha, nhorman, chrisp, John W. Linville,
Linux Netdev List
In-Reply-To: <20180927153204.GE12979@lunn.ch>
On Thu, Sep 27, 2018 at 6:34 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Driver return 256 bytes (reading it correctly, I verified it, no overruns),
> > however the extra bytes are presented due to this bug (expecting to parse
> > 640 bytes).
> >
> > Do you see another bug here? Am I missing something?
>
> Hi Erin
Eran...
>
> Please could you try ethtool -m raw on so you get a binary dump. The
> file which Chris provided had more bytes in it than 256.
I ran '-m raw on' on QSFP28.
File size is 256 bytes.
(with and without my suggested patch...)
Eran
>
> Thanks
> Andrew
^ permalink raw reply
* Re: [PATCH net-next v6 01/23] asm: simd context helper API
From: Ard Biesheuvel @ 2018-10-02 7:18 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Andy Lutomirski, Joe Perches, LKML, Netdev,
Linux Crypto Mailing List, David Miller, Greg Kroah-Hartman,
Samuel Neves, Andrew Lutomirski, Thomas Gleixner, linux-arch
In-Reply-To: <CAHmME9o7_Xh=dZfnEHbPCJdqzXW1L5R7qjg=_sqPZiP2KmTc1Q@mail.gmail.com>
On 1 October 2018 at 03:43, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Sun, Sep 30, 2018 at 7:35 AM Andy Lutomirski <luto@amacapital.net> wrote:
>> >>>>>>> Oh, and another thing (and I'm surprised checkpatch.pl didn't complain
>> >>>>>>> about it): the use of typedef in new code is strongly discouraged.
>> >>>>>>> This policy predates my involvement, so perhaps Joe can elaborate on
>> >>>>>>> the rationale?
>> >>>>>>
>> >>>>>> In case it matters, the motivation for making this a typedef is I
>> >>>>>> could imagine this at some point turning into a more complicated
>> >>>>>> struct on certain platforms and that would make refactoring easier. I
>> >>>>>> could just make it `struct simd_context` now with 1 member though...
>> >>>>>
>> >>>>> Yes that makes sense
>> >>>>
>> >>>> The rationale for it being a typedef or moving to a struct now?
>> >>>
>> >>> Yes just switch to a struct.
>> >>
>> >> Okay. No problem with that, but will wait to hear from Joe first.
>> >
>> > Why do you need to hear from me again?
>> >
>> > As far as I know, the only info about typedef avoidance are in
>> > Documentation/process/coding-style.rst section 5.
>> >
>> >
>>
>> I personally prefer it with the typedef. If this were my code, I’d say the coding style is silly for opaque tiny structs like this.
>
> I'll stick with a typedef. Reading the style guide, this clearly falls
> into 5.a, 5.b, and maybe 5.c. For 5.a, at some point this will
> possibly contain architecture specific blobs. For 5.b, it is just an
> enum (integer) right now.
I can live with that, but other may still object.
In any case, since you are using the enum member as a bitfield, it
would be better to typedef it to int instead, and retain the enum
definition only for the symbolic constants.
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: Fix uninitialized variable use at qsfp dump
From: Eran Ben Elisha @ 2018-10-02 7:17 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev@vger.kernel.org, John W. Linville, Chris Preimesberger,
Neil Horman, Vidya Sagar Ravipati
In-Reply-To: <20180927160817.GH12979@lunn.ch>
On 9/27/2018 7:08 PM, Andrew Lunn wrote:
> On Thu, Sep 27, 2018 at 04:48:35PM +0300, Eran Ben Elisha wrote:
>> Struct sff_diags can be used uninitialized at sff8636_show_dom, this
>> caused the tool to show unreported fields (supports_alarms) by the lower
>> level driver.
>>
>> Fixes: a5e73bb05ee4 ("ethtool:QSFP Plus/QSFP28 Diagnostics Information Support")
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> ---
>> qsfp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/qsfp.c b/qsfp.c
>> index 32e195d12dc0..d196aa1753de 100644
>> --- a/qsfp.c
>> +++ b/qsfp.c
>> @@ -671,7 +671,7 @@ static void sff8636_dom_parse(const __u8 *id, struct sff_diags *sd)
>>
>> static void sff8636_show_dom(const __u8 *id, __u32 eeprom_len)
>> {
>> - struct sff_diags sd;
>> + struct sff_diags sd = {0};
>> char *rx_power_string = NULL;
>> char power_string[MAX_DESC_SIZE];
>> int i;
>
> Hi Erin
>
> I wonder if similar bugs happen in sfpdiag.c?
Bug is not present here, all sd needed fields are being filled in
sff8472_parse_eeprom right at the beginning.
>
> void sff8472_show_all(const __u8 *id)
> {
> struct sff_diags sd;
> char *rx_power_string = NULL;
> int i;
>
> sd is also on the stack and not initialised. Could you expand your
> patch to also clear this one as well, just to be safe.
Although bug is not present, I agree that better initialize it to be on
the safe side. will post soon.
Eran
>
> Thanks
> Andrew
>
^ permalink raw reply
* [PATCH ethtool v2] ethtool: Fix uninitialized variable use at qsfp dump
From: Eran Ben Elisha @ 2018-10-02 7:24 UTC (permalink / raw)
To: netdev, John W. Linville
Cc: Chris Preimesberger, Andrew Lunn, Neil Horman,
Vidya Sagar Ravipati, Eran Ben Elisha
Struct sff_diags can be used uninitialized at sff8636_show_dom, this
caused the tool to show unreported fields (supports_alarms) by the lower
level driver.
In addition, make sure the same struct is being initialized at
sff8472_parse_eeprom function, to avoid the same issue here.
Fixes: a5e73bb05ee4 ("ethtool:QSFP Plus/QSFP28 Diagnostics Information Support")
Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
---
qsfp.c | 2 +-
sfpdiag.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/qsfp.c b/qsfp.c
index 32e195d12dc0..d196aa1753de 100644
--- a/qsfp.c
+++ b/qsfp.c
@@ -671,7 +671,7 @@ static void sff8636_dom_parse(const __u8 *id, struct sff_diags *sd)
static void sff8636_show_dom(const __u8 *id, __u32 eeprom_len)
{
- struct sff_diags sd;
+ struct sff_diags sd = {0};
char *rx_power_string = NULL;
char power_string[MAX_DESC_SIZE];
int i;
diff --git a/sfpdiag.c b/sfpdiag.c
index 32e4cd8917a4..fa41651422ea 100644
--- a/sfpdiag.c
+++ b/sfpdiag.c
@@ -241,7 +241,7 @@ static void sff8472_parse_eeprom(const __u8 *id, struct sff_diags *sd)
void sff8472_show_all(const __u8 *id)
{
- struct sff_diags sd;
+ struct sff_diags sd = {0};
char *rx_power_string = NULL;
int i;
--
1.8.3.1
^ permalink raw reply related
* [PATCH iproute2/net-next v3] tc_util: Add support for showing TCA_STATS_BASIC_HW statistics
From: Eelco Chaudron @ 2018-10-02 7:27 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, dsahern
Add support for showing hardware specific counters to easy
troubleshooting hardware offload.
$ tc -s filter show dev enp3s0np0 parent ffff:
filter protocol ip pref 1 flower chain 0
filter protocol ip pref 1 flower chain 0 handle 0x1
eth_type ipv4
dst_ip 2.0.0.0
src_ip 1.0.0.0
ip_flags nofrag
in_hw
action order 1: mirred (Egress Redirect to device eth1) stolen
index 1 ref 1 bind 1 installed 0 sec used 0 sec
Action statistics:
Sent 534884742 bytes 8915697 pkt (dropped 0, overlimits 0 requeues 0)
Sent software 187542 bytes 4077 pkt
Sent hardware 534697200 bytes 8911620 pkt
backlog 0b 0p requeues 0
cookie 89173e6a44447001becfd486bda17e29
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v3:
* Resent as kernel side is now in
* Rebased on latest iproute2-next branch
v2:
* Removed unnecessary initialization
* Made not displaying of missing TCA_STATS_BASIC_HW more obvious
* Use _SL_ macro for single line output
tc/tc_util.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/tc/tc_util.c b/tc/tc_util.c
index cafbe49..a082c73 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -754,6 +754,44 @@ void print_tm(FILE *f, const struct tcf_t *tm)
}
}
+static void print_tcstats_basic_hw(struct rtattr **tbs, char *prefix)
+{
+ struct gnet_stats_basic bs_hw;
+
+ if (!tbs[TCA_STATS_BASIC_HW])
+ return;
+
+ memcpy(&bs_hw, RTA_DATA(tbs[TCA_STATS_BASIC_HW]),
+ MIN(RTA_PAYLOAD(tbs[TCA_STATS_BASIC_HW]), sizeof(bs_hw)));
+
+ if (bs_hw.bytes == 0 && bs_hw.packets == 0)
+ return;
+
+ if (tbs[TCA_STATS_BASIC]) {
+ struct gnet_stats_basic bs;
+
+ memcpy(&bs, RTA_DATA(tbs[TCA_STATS_BASIC]),
+ MIN(RTA_PAYLOAD(tbs[TCA_STATS_BASIC]),
+ sizeof(bs)));
+
+ if (bs.bytes >= bs_hw.bytes && bs.packets >= bs_hw.packets) {
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+ print_string(PRINT_FP, NULL, "%s", prefix);
+ print_lluint(PRINT_ANY, "sw_bytes",
+ "Sent software %llu bytes",
+ bs.bytes - bs_hw.bytes);
+ print_uint(PRINT_ANY, "sw_packets", " %u pkt",
+ bs.packets - bs_hw.packets);
+ }
+ }
+
+ print_string(PRINT_FP, NULL, "%s", _SL_);
+ print_string(PRINT_FP, NULL, "%s", prefix);
+ print_lluint(PRINT_ANY, "hw_bytes", "Sent hardware %llu bytes",
+ bs_hw.bytes);
+ print_uint(PRINT_ANY, "hw_packets", " %u pkt", bs_hw.packets);
+}
+
void print_tcstats2_attr(FILE *fp, struct rtattr *rta, char *prefix, struct rtattr **xstats)
{
SPRINT_BUF(b1);
@@ -780,6 +818,9 @@ void print_tcstats2_attr(FILE *fp, struct rtattr *rta, char *prefix, struct rtat
print_uint(PRINT_ANY, "requeues", " requeues %u) ", q.requeues);
}
+ if (tbs[TCA_STATS_BASIC_HW])
+ print_tcstats_basic_hw(tbs, prefix);
+
if (tbs[TCA_STATS_RATE_EST64]) {
struct gnet_stats_rate_est64 re = {0};
^ permalink raw reply related
* Re: [PATCH iproute2/net-next v2] tc_util: Add support for showing TCA_STATS_BASIC_HW statistics
From: Eelco Chaudron @ 2018-10-02 7:28 UTC (permalink / raw)
To: David Ahern; +Cc: Stephen Hemminger, netdev, davem
In-Reply-To: <35ca247c-e4dc-1653-a22b-c12bd8256a26@gmail.com>
On 1 Oct 2018, at 17:12, David Ahern wrote:
> On 10/1/18 4:29 AM, Eelco Chaudron wrote:
>>>> Hi Stephen, anything else required for this patch to be accepted?
>>>>
>>>> FYI the kernel side of this patch has been excepted on net-next.
>>>>
>>>> Cheers,
>>>>
>>>> Eelco
>>>
>>> David Ahern handles net-next see patchwork
>>> https://patchwork.ozlabs.org/patch/956225/
>>>
>>> I think he was just waiting for the kernel part to merge.
>>
>> Thanks for making me aware of the patchwork for iproute.
>>
>
> Now that the kernel side is in, please re-send the iproute2 patches.
Rebased the patch on the latest iproute2-next and sent a v3.
//Eelco
^ permalink raw reply
* Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module
From: Pablo Neira Ayuso @ 2018-10-02 7:59 UTC (permalink / raw)
To: Chenbo Feng
Cc: netdev, netfilter-devel, kernel-team, Lorenzo Colitti, maze,
Chenbo Feng
In-Reply-To: <1538443388-6881-3-git-send-email-chenbofeng.kernel@gmail.com>
Hi,
On Mon, Oct 01, 2018 at 06:23:08PM -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> A major flaw of the current xt_quota module is that quota in a specific
> rule gets reset every time there is a rule change in the same table. It
> makes the xt_quota module not very useful in a table in which iptables
> rules are changed at run time. This fix introduces a new counter that is
> visible to userspace as the remaining quota of the current rule. When
> userspace restores the rules in a table, it can restore the counter to
> the remaining quota instead of resetting it to the full quota.
A few questions, see below.
First one is, don't we need a new match revision for this new option?
> Signed-off-by: Chenbo Feng <fengc@google.com>
> Suggested-by: Maciej Żenczykowski <maze@google.com>
> Reviewed-by: Maciej Żenczykowski <maze@google.com>
> ---
> include/uapi/linux/netfilter/xt_quota.h | 8 +++--
> net/netfilter/xt_quota.c | 55 +++++++++++++--------------------
> 2 files changed, 27 insertions(+), 36 deletions(-)
>
> diff --git a/include/uapi/linux/netfilter/xt_quota.h b/include/uapi/linux/netfilter/xt_quota.h
> index f3ba5d9..d72fd52 100644
> --- a/include/uapi/linux/netfilter/xt_quota.h
> +++ b/include/uapi/linux/netfilter/xt_quota.h
> @@ -15,9 +15,11 @@ struct xt_quota_info {
> __u32 flags;
> __u32 pad;
> __aligned_u64 quota;
> -
> - /* Used internally by the kernel */
> - struct xt_quota_priv *master;
> +#ifdef __KERNEL__
> + atomic64_t counter;
> +#else
> + __aligned_u64 remain;
> +#endif
> };
>
> #endif /* _XT_QUOTA_H */
> diff --git a/net/netfilter/xt_quota.c b/net/netfilter/xt_quota.c
> index 10d61a6..6afa7f4 100644
> --- a/net/netfilter/xt_quota.c
> +++ b/net/netfilter/xt_quota.c
> @@ -11,11 +11,6 @@
> #include <linux/netfilter/xt_quota.h>
> #include <linux/module.h>
>
> -struct xt_quota_priv {
> - spinlock_t lock;
> - uint64_t quota;
> -};
> -
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Sam Johnston <samj@samj.net>");
> MODULE_DESCRIPTION("Xtables: countdown quota match");
> @@ -26,54 +21,48 @@ static bool
> quota_mt(const struct sk_buff *skb, struct xt_action_param *par)
> {
> struct xt_quota_info *q = (void *)par->matchinfo;
> - struct xt_quota_priv *priv = q->master;
> + u64 current_count = atomic64_read(&q->counter);
> bool ret = q->flags & XT_QUOTA_INVERT;
> -
> - spin_lock_bh(&priv->lock);
> - if (priv->quota >= skb->len) {
> - priv->quota -= skb->len;
> - ret = !ret;
> - } else {
> - /* we do not allow even small packets from now on */
> - priv->quota = 0;
> - }
> - spin_unlock_bh(&priv->lock);
> -
> - return ret;
> + u64 old_count, new_count;
> +
> + do {
> + if (current_count == 1)
> + return ret;
So 1 means, don't keep updating, quota is depleted?
This current_count = 1 would be exposed to userspace too, right?
Hm, this semantics are going to be a bit awkwards to users I think, I
would prefer to expose this in a different way.
> + if (current_count <= skb->len) {
> + atomic64_set(&q->counter, 1);
> + return ret;
> + }
> + old_count = current_count;
> + new_count = current_count - skb->len;
> + current_count = atomic64_cmpxchg(&q->counter, old_count,
> + new_count);
> + } while (current_count != old_count);
Probably we simplify this via atomic64_add_return()?
I guess problem is userspace may get a current counter that is larger
than the quota, but we could handle this from userspace iptables to
print a value that equals the quota, ie. from userspace, before
printing:
if (consumed > quota)
printf("--consumed %PRIu64 ", quota);
else
printf("--consumed %PRIu64 ", consumed);
> + return !ret;
> }
Thanks !
^ permalink raw reply
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