* Re: [PATCH net-next] tcp: Remove some spurious dropped packet profile hits from the passive connection accept path
From: Eric Dumazet @ 2014-11-20 20:47 UTC (permalink / raw)
To: Rick Jones; +Cc: netdev, davem
In-Reply-To: <20141120185829.986CB290095D@tardy>
On Thu, 2014-11-20 at 10:58 -0800, Rick Jones wrote:
> From: Rick Jones <rick.jones2@hp.com>
>
> When a system is the passive accepter of many connections, for example
> when the target of a netperf TCP_CC or TCP_CRR test, or as say a web
> server, the discard of the skb containing the TCP SYN being processed
> for the LISTEN endpoint should be a consume_skb() rather than a kfree_skb()
> to avoid cluttering a dropped packet profile.
>
> Signed-off-by: Rick Jones <rick.jones2@hp.com>
>
> ---
So what happens if we really drop the packet ?
TCP stack at this point owns the packet, it is possible to mark a bit in
it to either call consume_skb() or kfree_skb()
I attempted this once but gave up because it was a quite intrusive
patch...
^ permalink raw reply
* Re: [PATCH net-next] filter: add bpf_optimize_div()
From: Alexei Starovoitov @ 2014-11-20 21:07 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: Network Development
In-Reply-To: <1416512551-22252-1-git-send-email-kda@linux-powerpc.org>
On Thu, Nov 20, 2014 at 11:42 AM, Denis Kirjanov <kda@linux-powerpc.org> wrote:
> optimize_div() found in mips bpf jit is really usefull
> for other arches. So let's put it in filter.h
>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
> ---
> include/linux/filter.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index ca95abd..b385637 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> +
> +static inline int bpf_optimize_div(u32 *k)
'inline' is unnecessary
> +{
> + /* power of 2 divides can be implemented with right shift */
> + if (!(*k & (*k-1))) {
> + *k = ilog2(*k);
> + return 1;
> + }
> +
> + return 0;
> +}
I don't think it makes sense to add a helper function
without first user. If you really want to use it
in ppc, make this change as part of the series.
In the 1st patch move it out of mips and use it in
mips jit and 2nd patch use it in ppc.
Also since you mentioned mips...
if (k == 1 || optimize_div(&k)) {
ctx->flags |= SEEN_A;
emit_jit_reg_move(r_A, r_zero, ctx);
this is definitely wrong in there.
Also with such helper function we'd need to be
careful not to modify 'k' in place, otherwise
multi-pass jit algorithm would be broken.
In general I don't think such optimizations
belong in kernel at all. User space should be doing this.
^ permalink raw reply
* pull request: wireless 2014-11-20
From: John W. Linville @ 2014-11-20 21:00 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6471 bytes --]
Dave,
Please full this little batch of fixes intended for the 3.18 stream!
For the mac80211 patch, Johannes says:
"Here's another last minute fix, for minstrel HT crashing
depending on the value of some uninitialised stack."
On top of that...
Ben Greear fixes an ath9k regression in which a BSSID mask is
miscalculated.
Dmitry Torokhov corrects an error handling routing in brcmfmac which
was checking an unsigned variable for a negative value.
Johannes Berg avoids a build problem in brcmfmac for arches where
linux/unaligned/access_ok.h and asm/unaligned.h conflict.
Mathy Vanhoef addresses another brcmfmac issue so as to eliminate a
use-after-free of the URB transfer buffer if a timeout occurs.
Please let me know if there are problems!
Thanks,
John
---
The following changes since commit 4e6ce4dc7ce71d0886908d55129d5d6482a27ff9:
ath9k: Fix RTC_DERIVED_CLK usage (2014-11-11 16:24:18 -0500)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git tags/master-2014-11-20
for you to fetch changes up to a1d69c60c44134f64945bbf6a6dfda22eaf4a214:
brcmfmac: don't include linux/unaligned/access_ok.h (2014-11-20 14:46:45 -0500)
----------------------------------------------------------------
Ben Greear (1):
ath9k: fix regression in bssidmask calculation
Dmitry Torokhov (1):
brcmfmac: fix error handling of irq_of_parse_and_map
Felix Fietkau (1):
mac80211: minstrel_ht: fix a crash in rate sorting
Johannes Berg (1):
brcmfmac: don't include linux/unaligned/access_ok.h
John W. Linville (1):
Merge tag 'mac80211-for-john-2014-11-18' of git://git.kernel.org/.../jberg/mac80211
Mathy Vanhoef (1):
brcmfmac: kill URB when request timed out
drivers/net/wireless/ath/ath9k/main.c | 9 ++++++---
drivers/net/wireless/brcm80211/brcmfmac/of.c | 4 ++--
drivers/net/wireless/brcm80211/brcmfmac/pcie.c | 2 +-
drivers/net/wireless/brcm80211/brcmfmac/usb.c | 6 ++++--
net/mac80211/rc80211_minstrel_ht.c | 15 ++++++---------
5 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 30c66dfcd7a0..4f18a6be0c7d 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -974,9 +974,8 @@ void ath9k_calculate_iter_data(struct ath_softc *sc,
struct ath_vif *avp;
/*
- * Pick the MAC address of the first interface as the new hardware
- * MAC address. The hardware will use it together with the BSSID mask
- * when matching addresses.
+ * The hardware will use primary station addr together with the
+ * BSSID mask when matching addresses.
*/
memset(iter_data, 0, sizeof(*iter_data));
memset(&iter_data->mask, 0xff, ETH_ALEN);
@@ -1205,6 +1204,8 @@ static int ath9k_add_interface(struct ieee80211_hw *hw,
list_add_tail(&avp->list, &avp->chanctx->vifs);
}
+ ath9k_calculate_summary_state(sc, avp->chanctx);
+
ath9k_assign_hw_queues(hw, vif);
an->sc = sc;
@@ -1274,6 +1275,8 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
ath_tx_node_cleanup(sc, &avp->mcast_node);
+ ath9k_calculate_summary_state(sc, avp->chanctx);
+
mutex_unlock(&sc->mutex);
}
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/of.c b/drivers/net/wireless/brcm80211/brcmfmac/of.c
index f05f5270fec1..927bffd5be64 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/of.c
@@ -40,8 +40,8 @@ void brcmf_of_probe(struct brcmf_sdio_dev *sdiodev)
return;
irq = irq_of_parse_and_map(np, 0);
- if (irq < 0) {
- brcmf_err("interrupt could not be mapped: err=%d\n", irq);
+ if (!irq) {
+ brcmf_err("interrupt could not be mapped\n");
devm_kfree(dev, sdiodev->pdata);
return;
}
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
index 8c0632ec9f7a..16fef3382019 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/pcie.c
@@ -19,10 +19,10 @@
#include <linux/pci.h>
#include <linux/vmalloc.h>
#include <linux/delay.h>
-#include <linux/unaligned/access_ok.h>
#include <linux/interrupt.h>
#include <linux/bcma/bcma.h>
#include <linux/sched.h>
+#include <asm/unaligned.h>
#include <soc.h>
#include <chipcommon.h>
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
index dc135915470d..875d1142c8b0 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
@@ -669,10 +669,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
goto finalize;
}
- if (!brcmf_usb_ioctl_resp_wait(devinfo))
+ if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
+ usb_kill_urb(devinfo->ctl_urb);
ret = -ETIMEDOUT;
- else
+ } else {
memcpy(buffer, tmpbuf, buflen);
+ }
finalize:
kfree(tmpbuf);
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index df90ce2db00c..408fd8ab4eef 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -252,19 +252,16 @@ minstrel_ht_sort_best_tp_rates(struct minstrel_ht_sta *mi, u8 index,
cur_thr = mi->groups[cur_group].rates[cur_idx].cur_tp;
cur_prob = mi->groups[cur_group].rates[cur_idx].probability;
- tmp_group = tp_list[j - 1] / MCS_GROUP_RATES;
- tmp_idx = tp_list[j - 1] % MCS_GROUP_RATES;
- tmp_thr = mi->groups[tmp_group].rates[tmp_idx].cur_tp;
- tmp_prob = mi->groups[tmp_group].rates[tmp_idx].probability;
-
- while (j > 0 && (cur_thr > tmp_thr ||
- (cur_thr == tmp_thr && cur_prob > tmp_prob))) {
- j--;
+ do {
tmp_group = tp_list[j - 1] / MCS_GROUP_RATES;
tmp_idx = tp_list[j - 1] % MCS_GROUP_RATES;
tmp_thr = mi->groups[tmp_group].rates[tmp_idx].cur_tp;
tmp_prob = mi->groups[tmp_group].rates[tmp_idx].probability;
- }
+ if (cur_thr < tmp_thr ||
+ (cur_thr == tmp_thr && cur_prob <= tmp_prob))
+ break;
+ j--;
+ } while (j > 0);
if (j < MAX_THR_RATES - 1) {
memmove(&tp_list[j + 1], &tp_list[j], (sizeof(*tp_list) *
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply related
* Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.
From: Keller, Jacob E @ 2014-11-20 21:28 UTC (permalink / raw)
To: richardcochran@gmail.com
Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <20141120091804.GA4587@localhost.localdomain>
On Thu, 2014-11-20 at 10:18 +0100, Richard Cochran wrote:
> On Wed, Nov 19, 2014 at 09:06:19PM +0000, Keller, Jacob E wrote:
> > On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote:
> > > On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote:
> > > > Good catch :)
>
> I have not been able to reproduce the crash, and so the cause is not
> what I thought it was. Maybe it was my patch that preserved the
> enabled interrupts in igb_ptp_reset(). I didn't notice that the driver
> frees and reallocates the ptp_clock. I would never do that, myself.
>
Yea, I think that was a design I tried in the ixgbe driver, but it
really isn't good.
> > I think you need something here, but it should be clearing that register
> > after a MAC reset, so it needs to be re-initialized. I'm not sure if
> > that reset path was used in the same place in the past.
>
> Okay, lets figure this out. Why is there a PTP reset function at all?
> I don't know, lets see who calls it...
>
> Finding functions calling: igb_ptp_reset
> ----------------------------------------
> *** drivers/net/ethernet/intel/igb/igb_main.c:
> igb_reset[2033] igb_ptp_reset(adapter);
>
> Easy enough to understand. But who is calling igb_reset?
>
> Finding functions calling: igb_reset
> ------------------------------------
> *** drivers/net/ethernet/intel/igb/igb_ethtool.c:
> igb_set_settings[345] igb_reset(adapter);
> igb_set_pauseparam[409] igb_reset(adapter);
> igb_diag_test[2016] igb_reset(adapter);
> igb_set_eee[2729] igb_reset(adapter);
> *** drivers/net/ethernet/intel/igb/igb_main.c:
> igb_down[1814] igb_reset(adapter);
> igb_set_features[2069] igb_reset(adapter);
> igb_probe[2526] igb_reset(adapter);
> __igb_open[3110] igb_reset(adapter);
> igb_watchdog_task[4231] igb_reset(adapter);
> igb_change_mtu[5189] igb_reset(adapter);
> igb_resume[7460] igb_reset(adapter);
> igb_sriov_reinit[7545] igb_reset(adapter);
> igb_io_slot_reset[7678] igb_reset(adapter);
>
> Wow, that is quite much. So, whenever any random parameter is changed,
> we reset the PTP clock. Great.
Here is the problem. In igb_reset, we call e1000_reset_hw() which will
eventually perform a hardware MAC reset. Yes maybe we shouldn't be
calling igb_reset everywhere.. I cannot answer that, however...
>
> Really, wouldn't better to reset the clock functions only when
> absolutely necessary?
>
We *are*. If e1000_reset_hw(hw) is called, the MAC registers will be
reset to their initial values, which includes not having SYSTIME setup,
and not having the outputs configured. There is not much that can be
done to avoid that besides major refactoring of the igb driver to avoid
resetting as much as it does. In most of those cases, I think the reset
is fine, and we actually aren't going to reset that many times during
nominal operation.
Some of the ethtool settings maybe could avoid resets, but I am not
certain during exactly which path they end up calling resets.
Regards,
Jake
> Thanks,
> Richard
^ permalink raw reply
* Re: [RFC] net: ipv4: drop unicast encapsulated in L2 multicast
From: Johannes Berg @ 2014-11-20 21:31 UTC (permalink / raw)
To: David Miller
Cc: ja-FgGsKACvmQM, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20140822.105405.1982870131653082781.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Fri, 2014-08-22 at 10:54 -0700, David Miller wrote:
> >> if (res.type == RTN_BROADCAST)
> >> goto brd_input;
> >
> > Is this place better, after checking for RTN_BROADCAST?
> >
> > /* ARP link-layer broadcasts are acceptable here */
> > if ((skb->pkt_type == PACKET_BROADCAST ||
> > skb->pkt_type == PACKET_MULTICAST) &&
> > skb->protocol == htons(ETH_P_IP))
> > goto e_inval;
>
> Indeed, this would make ARP happier, but that still leaves open the
> issue of CLUSTERIP.
I'm back looking at this, but must admit I'm completely confused now :-)
I could add an IPv4 sysctl to control this behaviour:
0 - off
1 - RFC 1122 "SHOULD"
2 - also drop unicast-in-multicast (for wireless)
But I guess due to cluster-IP it would have to default to 0.
However, talk about ip_local_deliver_finish() in this thread has me
wondering if we could just implement it using iptables? I guess
ipt_addrtype and ip6t_addrtype would let me do that?
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next] tcp: Remove some spurious dropped packet profile hits from the passive connection accept path
From: Rick Jones @ 2014-11-20 21:31 UTC (permalink / raw)
To: Eric Dumazet, Rick Jones; +Cc: netdev, davem
In-Reply-To: <1416516423.8629.43.camel@edumazet-glaptop2.roam.corp.google.com>
On 11/20/2014 12:47 PM, Eric Dumazet wrote:
> On Thu, 2014-11-20 at 10:58 -0800, Rick Jones wrote:
>> From: Rick Jones <rick.jones2@hp.com>
>>
>> When a system is the passive accepter of many connections, for example
>> when the target of a netperf TCP_CC or TCP_CRR test, or as say a web
>> server, the discard of the skb containing the TCP SYN being processed
>> for the LISTEN endpoint should be a consume_skb() rather than a kfree_skb()
>> to avoid cluttering a dropped packet profile.
>>
>> Signed-off-by: Rick Jones <rick.jones2@hp.com>
>>
>> ---
>
> So what happens if we really drop the packet ?
Do you mean when there is actually data in the SYN?
rick
>
> TCP stack at this point owns the packet, it is possible to mark a bit in
> it to either call consume_skb() or kfree_skb()
>
> I attempted this once but gave up because it was a quite intrusive
> patch...
^ permalink raw reply
* Re: [PATCH net-next] filter: add bpf_optimize_div()
From: Daniel Borkmann @ 2014-11-20 21:32 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: netdev, Alexei Starovoitov
In-Reply-To: <1416512551-22252-1-git-send-email-kda@linux-powerpc.org>
On 11/20/2014 08:42 PM, Denis Kirjanov wrote:
> optimize_div() found in mips bpf jit is really usefull
> for other arches. So let's put it in filter.h
>
> CC: Alexei Starovoitov <ast@plumgrid.com>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
NAK
1) There's no user of this function (you name MIPS JIT, but
don't remove it from there) ...
2) It's not really specific or tied to the BPF API in particular,
so doesn't really belong here, rather some more generic kernel
header, iff anything.
> ---
> include/linux/filter.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index ca95abd..b385637 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -414,6 +414,17 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
> print_hex_dump(KERN_ERR, "JIT code: ", DUMP_PREFIX_OFFSET,
> 16, 1, image, proglen, false);
> }
> +
> +static inline int bpf_optimize_div(u32 *k)
> +{
> + /* power of 2 divides can be implemented with right shift */
> + if (!(*k & (*k-1))) {
> + *k = ilog2(*k);
> + return 1;
> + }
> +
> + return 0;
> +}
> #else
> static inline void bpf_jit_compile(struct bpf_prog *fp)
> {
>
^ permalink raw reply
* Re: [RFC] situation with csum_and_copy_... API
From: Al Viro @ 2014-11-20 21:47 UTC (permalink / raw)
To: David Miller
Cc: torvalds, netdev, linux-kernel, target-devel,
Nicholas A. Bellinger, Christoph Hellwig
In-Reply-To: <20141119.165340.2162829993279387495.davem@davemloft.net>
On Wed, Nov 19, 2014 at 04:53:40PM -0500, David Miller wrote:
> Pulled, thanks Al.
Umm... Not in net-next.git#master... Anyway, the next portion is in
vfs.git#iov_iter-net right now; I'll post it on netdev once I get some
sleep.
It's getting close to really interesting parts. Right now the main obstacle
is in iscsit_do_rx_data/iscsit_do_tx_data; what happens there is reuse of
iovec if kernel_sendmsg() gives a short write - it tries to send again, with
the same iovec and decremented length. Ditto on RX side (with kernel_recvmsg(),
obviously).
As far as I can see, these retries on the send side are simply broken -
normally we are talking to TCP sockets there and tcp_sendmsg() does *not*
modify iovec in normal case. IOW, if you get 8K sent out of 80K, the next
time it'll try to send 72K - already sent piece + 64K following it, etc.
Could target-devel folks tell how realistic those resends are, in the
first place? Both with TX and RX sides... Is there any sane limit on
iovec size there, etc.
Note that while conversion to iov_iter will provide a very simple solution
(iovec remains unchanged, iterator advances and we just need to avoid
reinitializing it for subsequent iterations in those loops), it won't solve
the problem in older kernels; that code had been there since 2011 and
iov_iter conversion is far too invasive for -stable.
^ permalink raw reply
* Re: [PATCH net-next] tcp: Remove some spurious dropped packet profile hits from the passive connection accept path
From: Eric Dumazet @ 2014-11-20 21:48 UTC (permalink / raw)
To: Rick Jones; +Cc: Rick Jones, netdev, davem
In-Reply-To: <546E5DAF.5090907@hp.com>
On Thu, 2014-11-20 at 13:31 -0800, Rick Jones wrote:
> Do you mean when there is actually data in the SYN?
No, I mean the packet can be _dropped_ here, really.
SYN flood for example.
^ permalink raw reply
* Re: [PATCH net-next] tcp: Remove some spurious dropped packet profile hits from the passive connection accept path
From: Rick Jones @ 2014-11-20 21:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem
In-Reply-To: <1416520109.8629.44.camel@edumazet-glaptop2.roam.corp.google.com>
On 11/20/2014 01:48 PM, Eric Dumazet wrote:
> On Thu, 2014-11-20 at 13:31 -0800, Rick Jones wrote:
>
>> Do you mean when there is actually data in the SYN?
>
> No, I mean the packet can be _dropped_ here, really.
>
> SYN flood for example.
Ah. I did not pick-up on that. Seeing the kfree_skb() in the original
code I thought it was the end of the line for the segment.
rick
^ permalink raw reply
* Re: [RFC] situation with csum_and_copy_... API
From: Eric Dumazet @ 2014-11-20 21:55 UTC (permalink / raw)
To: Al Viro
Cc: David Miller, torvalds, netdev, linux-kernel, target-devel,
Nicholas A. Bellinger, Christoph Hellwig
In-Reply-To: <20141120214753.GR7996@ZenIV.linux.org.uk>
On Thu, 2014-11-20 at 21:47 +0000, Al Viro wrote:
> As far as I can see, these retries on the send side are simply broken -
> normally we are talking to TCP sockets there and tcp_sendmsg() does *not*
> modify iovec in normal case.
Arg... I sent this morning something doing this (against net-next tree)
Is it a problem ?
Or can we consider FASTOPEN being not normal case ? ;)
https://patchwork.ozlabs.org/patch/412776/
^ permalink raw reply
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Cong Wang @ 2014-11-20 21:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wengang, netdev
In-Reply-To: <1416516104.8629.39.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, Nov 20, 2014 at 12:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-11-20 at 09:34 -0800, Cong Wang wrote:
>
>> I didn't look into ipoib_header_ops, thought it might have some dependency
>> on symbols.
>
> I did look before answering and suggesting this, you really should do
> the same instead of giving advices of over engineering the stack.
>
> Best is the enemy of the good.
>
> Its hard to find some networking function trivial than this one.
What about other modules defining *header_ops? Don't they
need to move to vmlinux as well?
I still don't like this workaround even just for stable. Although
definitely a real fix could be harder to backport, for me it is normal
backport 8+ patches to stable:
http://www.spinics.net/lists/stable/msg66122.html
http://www.spinics.net/lists/linux-fsdevel/msg79967.html
I know you disagree, I don't even want to waste time on arguing it.
^ permalink raw reply
* Re: [PATCH net-next] filter: add bpf_optimize_div()
From: Daniel Borkmann @ 2014-11-20 21:57 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Denis Kirjanov, Network Development
In-Reply-To: <CAMEtUuw1q_=CuZOHqd1WDd8ZE+bo-CNsbQ4rFPgRRVy7H1Q4uA@mail.gmail.com>
On 11/20/2014 10:07 PM, Alexei Starovoitov wrote:
> On Thu, Nov 20, 2014 at 11:42 AM, Denis Kirjanov <kda@linux-powerpc.org> wrote:
...
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index ca95abd..b385637 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> +
>> +static inline int bpf_optimize_div(u32 *k)
>
> 'inline' is unnecessary
Btw, it's a header file here, so it's necessary here.
...
> In general I don't think such optimizations
> belong in kernel at all. User space should be doing this.
+1
^ permalink raw reply
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Eric Dumazet @ 2014-11-20 22:03 UTC (permalink / raw)
To: Cong Wang; +Cc: Wengang, netdev
In-Reply-To: <CAHA+R7Me4zPdGpeQVHFLLGVq=4cVar09jQFyjA81nxF_Fmuppw@mail.gmail.com>
On Thu, 2014-11-20 at 13:57 -0800, Cong Wang wrote:
> On Thu, Nov 20, 2014 at 12:41 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2014-11-20 at 09:34 -0800, Cong Wang wrote:
> >
> >> I didn't look into ipoib_header_ops, thought it might have some dependency
> >> on symbols.
> >
> > I did look before answering and suggesting this, you really should do
> > the same instead of giving advices of over engineering the stack.
> >
> > Best is the enemy of the good.
> >
> > Its hard to find some networking function trivial than this one.
>
> What about other modules defining *header_ops? Don't they
> need to move to vmlinux as well?
Yep, if they can be in a bonding device, for practical reasons, not to
prove your point.
>
> I still don't like this workaround even just for stable. Although
> definitely a real fix could be harder to backport, for me it is normal
> backport 8+ patches to stable:
>
> http://www.spinics.net/lists/stable/msg66122.html
> http://www.spinics.net/lists/linux-fsdevel/msg79967.html
>
> I know you disagree, I don't even want to waste time on arguing it.
Whatever, I really don't care.
Do your stuff, but don't ask people asking for an easy fix to do the
heart surgery.
Provide a patch, please.
^ permalink raw reply
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Cong Wang @ 2014-11-20 22:13 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wengang, netdev
In-Reply-To: <1416521035.8629.49.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, Nov 20, 2014 at 2:03 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Provide a patch, please.
>
>
Don't blame me.
I want to provide a real fix, you want a minimum fix for stable.
We agree that we disagree on this point, right? What's
more, according to your rule, I should yield to you when I
touch something you want to touch.
Also, no one seems to care about my previous question:
why only bonding has the problem?
^ permalink raw reply
* Re: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
From: Bjorn Helgaas @ 2014-11-20 22:21 UTC (permalink / raw)
To: Sathya Perla
Cc: linux-pci, netdev, ariel.elior, linux.nics, shahed.shaikh,
ddutile, Jeff Kirsher
In-Reply-To: <1415620410-4937-1-git-send-email-sathya.perla@emulex.com>
[+cc Jeff]
On Mon, Nov 10, 2014 at 05:23:26PM +0530, Sathya Perla wrote:
> A user must not be allowed to disable VFs while they are already assigned to
> a guest. This check is being made in each individual driver that implements
> the sriov_configure PCI method.
> This patch-set fixes this code duplication by moving this check from
> drivers to the sriov_nuvfs_store() routine just before invoking
> sriov_configure() when num_vfs is equal to 0.
>
> Vasundhara Volam (4):
> pci: move pci_assivned_vfs() check while disabling VFs to pci
> sub-system
> bnx2x: remove pci_assigned_vfs() check while disabling VFs
> i40e: remove pci_assigned_vfs() check while disabling VFs
> qlcnic: remove pci_assigned_vfs() check while disabling VFs
>
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 7 +------
> .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c | 10 ----------
> drivers/pci/pci-sysfs.c | 5 +++++
> 4 files changed, 7 insertions(+), 17 deletions(-)
I'm dropping these for the reasons Don & Alex outlined -- they don't fix
the problem for VFIO, so this amounts to shuffling around code that's known
to be broken, which seems more confusing than worthwhile.
Jeff, if I were you I would drop the i40e patch. I don't think it makes
sense to remove the check from i40e before adding it to the PCI core.
Bjorn
^ permalink raw reply
* Re: [RFC] situation with csum_and_copy_... API
From: Al Viro @ 2014-11-20 22:25 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, torvalds, netdev, linux-kernel, target-devel,
Nicholas A. Bellinger, Christoph Hellwig
In-Reply-To: <1416520542.8629.46.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, Nov 20, 2014 at 01:55:42PM -0800, Eric Dumazet wrote:
> On Thu, 2014-11-20 at 21:47 +0000, Al Viro wrote:
>
> > As far as I can see, these retries on the send side are simply broken -
> > normally we are talking to TCP sockets there and tcp_sendmsg() does *not*
> > modify iovec in normal case.
>
> Arg... I sent this morning something doing this (against net-next tree)
>
> Is it a problem ?
Yes, it is. You are breaking several _other_ kernel_sendmsg() users.
They are already slightly broken, but that'll make breakage much more
common.
Please, don't - the right thing to do is to have iov_iter in msghdr
(we already have the kernel and userland ones with different types and
we do not assume their layouts to be identical - currently they are,
but it's easy to change), keep iovec constant in all cases and advance
->msg_iter. Also in all cases.
Note that direct manipulations of what's currently in ->msg_iov are
wrong - all those loops over vector elements, etc., belong in low-level
primitives. The main missing ones right now are csum_and_copy_{from,to}_iter()
- I have those in local queue, but I'm still trying to get a reasonably
clean mm/iov_iter.c without ridiculous amounts of boilerplating. A bit more
massage is needed there...
Seriously, take a look at vfs.git#iov_iter-net; it's preparations for the
one that'll introduce ->msg_iter. Right now that branch has local iov_iter
declared and initialized in several ->sendmsg() and ->recvmsg() instances and
fed to primitives that work with it; after the conversion it'll be in
msg->msg_iter and it will be initialized by sock_sendmsg()/sock_recvmsg().
The tricky part is how to get through that without temporary breaking the
existing sendmsg/recvmsg users in the kernel *and* without a patch size from
hell. I more or less see how to carve the remaining steps into
reasonably-sized chunks; iscsi is one of the tricky ones and it, AFAICS,
is genuinely broken in mainline and will need fixes that can go into -stable.
And no, your solution doesn't work. Sorry. You'll break e.g. smb_send_kvec()
that way. ceph_tcp_sendmsg() as well, IIRC.
^ permalink raw reply
* Re: [linux-nics] [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
From: Jeff Kirsher @ 2014-11-20 22:42 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Sathya Perla, linux.nics, shahed.shaikh, linux-pci, ddutile,
ariel.elior, netdev
In-Reply-To: <20141120222127.GB7987@google.com>
[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]
On Thu, 2014-11-20 at 15:21 -0700, Bjorn Helgaas wrote:
> [+cc Jeff]
>
> On Mon, Nov 10, 2014 at 05:23:26PM +0530, Sathya Perla wrote:
> > A user must not be allowed to disable VFs while they are already assigned to
> > a guest. This check is being made in each individual driver that implements
> > the sriov_configure PCI method.
> > This patch-set fixes this code duplication by moving this check from
> > drivers to the sriov_nuvfs_store() routine just before invoking
> > sriov_configure() when num_vfs is equal to 0.
> >
> > Vasundhara Volam (4):
> > pci: move pci_assivned_vfs() check while disabling VFs to pci
> > sub-system
> > bnx2x: remove pci_assigned_vfs() check while disabling VFs
> > i40e: remove pci_assigned_vfs() check while disabling VFs
> > qlcnic: remove pci_assigned_vfs() check while disabling VFs
> >
> > drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 2 +-
> > drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 7 +------
> > .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c | 10 ----------
> > drivers/pci/pci-sysfs.c | 5 +++++
> > 4 files changed, 7 insertions(+), 17 deletions(-)
>
> I'm dropping these for the reasons Don & Alex outlined -- they don't fix
> the problem for VFIO, so this amounts to shuffling around code that's known
> to be broken, which seems more confusing than worthwhile.
>
> Jeff, if I were you I would drop the i40e patch. I don't think it makes
> sense to remove the check from i40e before adding it to the PCI core.
>
Thanks for the heads up, I will drop the i40e patch as well.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH net] cxgb4 : Fix DCB priority groups being returned in wrong order
From: Anish Bhatt @ 2014-11-20 22:46 UTC (permalink / raw)
To: netdev; +Cc: davem, hariprasad, Anish Bhatt
Peer priority groups were being reversed, but this was missed in the previous
fix sent out for this issue.
Fixes : ee7bc3cdc270 ('cxgb4 : dcb open-lldp interop fixes')
Signed-off-by: Anish Bhatt <anish@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.c
index cca6049..d8e60b3 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_dcb.c
@@ -1082,7 +1082,7 @@ static int cxgb4_cee_peer_getpg(struct net_device *dev, struct cee_pg *pg)
pgid = be32_to_cpu(pcmd.u.dcb.pgid.pgid);
for (i = 0; i < CXGB4_MAX_PRIORITY; i++)
- pg->prio_pg[i] = (pgid >> (i * 4)) & 0xF;
+ pg->prio_pg[7 - i] = (pgid >> (i * 4)) & 0xF;
INIT_PORT_DCB_READ_PEER_CMD(pcmd, pi->port_id);
pcmd.u.dcb.pgrate.type = FW_PORT_DCB_TYPE_PGRATE;
@@ -1094,7 +1094,8 @@ static int cxgb4_cee_peer_getpg(struct net_device *dev, struct cee_pg *pg)
}
for (i = 0; i < CXGB4_MAX_PRIORITY; i++)
- pg->pg_bw[i] = pcmd.u.dcb.pgrate.pgrate[i];
+ pg->pg_bw[7 - i] = pcmd.u.dcb.pgrate.pgrate[
+ (pgid >> (i * 4)) & 0xF];
return 0;
}
--
2.1.3
^ permalink raw reply related
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Jay Vosburgh @ 2014-11-20 22:53 UTC (permalink / raw)
To: Cong Wang; +Cc: Eric Dumazet, Wengang, netdev
In-Reply-To: <CAHA+R7P9XhGQZXFy6frxOyZJ4rX_MdVgY3kAPE1_wDNcvYd_yA@mail.gmail.com>
Cong Wang <cwang@twopensource.com> wrote:
>Also, no one seems to care about my previous question:
>why only bonding has the problem?
Bonding has the problem because it stashes a pointer to a data
structure (the header_ops) from another module, and when that module is
unloaded the dangling pointer may be dereferenced if it's not either
cleared or made to never go away.
Setting the bonding->header_ops to NULL (to avoid the current
problem with pktgen) has a race in dev_hard_header between where the
header_ops pointer is checked and where the ->create function is called.
This pointer business is the main reason the bonding path for
"not ARPHRD_ETHER" (i.e., ipoib) has extra complexity in the open/close
path, e.g.,
bond_slave_netdev_event():
[...]
switch (event) {
case NETDEV_UNREGISTER:
if (bond_dev->type != ARPHRD_ETHER)
bond_release_and_destroy(bond_dev, slave_dev);
else
bond_release(bond_dev, slave_dev);
If the ipoib ops were static in vmlinux, that would resolve the
pktgen problem, and also may eliminate the need for some of the ugly
bits like what I've pasted in above.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
* Re: [RFC] situation with csum_and_copy_... API
From: Eric Dumazet @ 2014-11-20 22:53 UTC (permalink / raw)
To: Al Viro
Cc: David Miller, torvalds, netdev, linux-kernel, target-devel,
Nicholas A. Bellinger, Christoph Hellwig
In-Reply-To: <20141120222506.GS7996@ZenIV.linux.org.uk>
On Thu, 2014-11-20 at 22:25 +0000, Al Viro wrote:
> Yes, it is. You are breaking several _other_ kernel_sendmsg() users.
> They are already slightly broken, but that'll make breakage much more
> common.
>
> Please, don't - the right thing to do is to have iov_iter in msghdr
> (we already have the kernel and userland ones with different types and
> we do not assume their layouts to be identical - currently they are,
> but it's easy to change), keep iovec constant in all cases and advance
> ->msg_iter. Also in all cases.
>
> Note that direct manipulations of what's currently in ->msg_iov are
> wrong - all those loops over vector elements, etc., belong in low-level
> primitives. The main missing ones right now are csum_and_copy_{from,to}_iter()
> - I have those in local queue, but I'm still trying to get a reasonably
> clean mm/iov_iter.c without ridiculous amounts of boilerplating. A bit more
> massage is needed there...
>
> Seriously, take a look at vfs.git#iov_iter-net; it's preparations for the
> one that'll introduce ->msg_iter. Right now that branch has local iov_iter
> declared and initialized in several ->sendmsg() and ->recvmsg() instances and
> fed to primitives that work with it; after the conversion it'll be in
> msg->msg_iter and it will be initialized by sock_sendmsg()/sock_recvmsg().
>
> The tricky part is how to get through that without temporary breaking the
> existing sendmsg/recvmsg users in the kernel *and* without a patch size from
> hell. I more or less see how to carve the remaining steps into
> reasonably-sized chunks; iscsi is one of the tricky ones and it, AFAICS,
> is genuinely broken in mainline and will need fixes that can go into -stable.
>
> And no, your solution doesn't work. Sorry. You'll break e.g. smb_send_kvec()
> that way. ceph_tcp_sendmsg() as well, IIRC.
Nowhere in tcp_sendmsg() the iov had const qualifier.
If it was declared as const, this discussion would not happen,
we would know we are not allowed to modify it.
iov_iter is nice, but not a single time it is used in net/
Please make sure to add const where appropriate.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] tcp: remove from tcp_sendmsg() some fastopen code
From: Eric Dumazet @ 2014-11-20 23:05 UTC (permalink / raw)
To: David Miller, Al Viro; +Cc: ycheng, nuclearcat, netdev, ncardwell
In-Reply-To: <1416500634.8629.31.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, 2014-11-20 at 08:23 -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> If we consume iovec bytes in tcp_send_syn_data(), we can remove
> annoying fastopen code in tcp_sendmsg() skipping over the already
> consumed bytes.
>
> Also add an unlikely(flags & MSG_FASTOPEN), as most TCP sendmsg() do not
> ask for FASTOPEN.
>
> Tested:
> Ran our 125 packetdrill fastopen tests
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
Please disregard this patch, Al Viro said sendmsg() was apparently not
allowed to change iovec.
Some callers depend on it staying constant.
Not clear why memcpy_fromiovec() even exists.
^ permalink raw reply
* [net-next v2 00/15][pull request] Intel Wired LAN Driver Updates 2014-11-20
From: Jeff Kirsher @ 2014-11-20 23:08 UTC (permalink / raw)
To: davem; +Cc: Jeff Kirsher, netdev, nhorman, sassmann, jogreene
This series contains updates to ixgbevf, i40e and i40evf.
Emil updates ixgbevf with much of the work that Alex Duyck did while at
Intel. First updates the driver to clear the status bits on allocation
instead of in the cleanup routine, this way we can leave the recieve
descriptor rings as a read only memory block until we actually have
buffers to give back to the hardware. Clean up ixgbevf_clean_rx_irq()
by creating ixgbevf_process_skb_field() to merge several similar
operations into this new function. Cleanup temporary variables within
the receive hot-path and reducing the scope of variables that do not
need to exist outside the main loop. Save on stack space by just
storing our updated values back in next_to_clean instead of using
a stack variable, which also collapses the size the function. Improve
performace on IOMMU enabled systems and reduce cache misses by changing
the basic receive patch for ixgbevf so that instead of receiving the
data into an skb, it is received into a double buffered page. Add
netpoll support by creating ixgbevf_netpoll(), which is a callback for
.ndo_poll_controller to allow for the VF interface to be used with
netconsole.
Mitch provides several cleanups and trivial fixes for i40e and i40evf.
First is a fix the overloading of the msg_size field in the
arq_event_info struct by splitting the field into two and renaming to
indicate the actual function of each field. Updates code comments
to match the actual function. Cleanup several checkpatch.pl warnings
by adding or removing blank lines, aligning function parameters, and
correcting over-long lines (which makes the code more readable).
Shannon provides a patch for i40e to write the extra bits that will
turn off the ITR wait for the interrupt, since we want the SW INT to
go off as soon as possible.
v2: updated patch 07 based on feedback from Alex Duyck by
- adding pfmemalloc check to a new function for reusable page
- moved atomic_inc outside of #if/else in ixgbevf_add_rx_frag()
- reverted the removal of the API check in ixgbevf_change_mtu()
The following are changes since commit daaf427c6ab392bedcd018e326b2ffa1e1110cd6:
bpf: fix arraymap NULL deref and missing overflow and zero size checks
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-next master
Emil Tantilov (9):
ixgbevf: Update ixgbevf_alloc_rx_buffers to handle clearing of status
bits
ixgbevf: Test Rx status bits directly out of the descriptor
ixgbevf: Combine the logic for post Rx processing into single function
ixgbevf: Cleanup variable usage, improve stack performance
ixgbevf: reorder main loop in ixgbe_clean_rx_irq to allow for
do/while/continue
ixgbevf: Update Rx next to clean in real time
ixgbevf: Change receive model to use double buffered page based
receives
ixgbevf: compare total_rx_packets and budget in ixgbevf_clean_rx_irq
ixgbevf: add netpoll support
Mitch Williams (5):
i40e: don't overload fields
i40evf: update header comments
i40evf: make checkpatch happy
i40evf: make comparisons consistent
i40evf: remove unnecessary else
Shannon Nelson (1):
i40e: trigger SW INT with no ITR wait
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 6 +-
drivers/net/ethernet/intel/i40e/i40e_adminq.h | 3 +-
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 5 +-
drivers/net/ethernet/intel/i40e/i40e_main.c | 17 +-
drivers/net/ethernet/intel/i40evf/i40e_adminq.c | 6 +-
drivers/net/ethernet/intel/i40evf/i40e_adminq.h | 3 +-
drivers/net/ethernet/intel/i40evf/i40evf.h | 2 +-
drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c | 14 +-
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 71 +-
.../net/ethernet/intel/i40evf/i40evf_virtchnl.c | 23 +-
drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 39 +-
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 720 ++++++++++++++-------
12 files changed, 595 insertions(+), 314 deletions(-)
--
1.9.3
^ permalink raw reply
* [net-next v2 02/15] ixgbevf: Test Rx status bits directly out of the descriptor
From: Jeff Kirsher @ 2014-11-20 23:09 UTC (permalink / raw)
To: davem
Cc: Emil Tantilov, netdev, nhorman, sassmann, jogreene,
Alexander Duyck, Jeff Kirsher
In-Reply-To: <1416524953-15161-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Emil Tantilov <emil.s.tantilov@intel.com>
Instead of keeping a local copy of the status bits from the descriptor
we can just read them directly - this is accomplished with the addition
of ixgbevf_test_staterr().
In addition instead of doing a byteswap on the status bits value, we
can byteswap the constant values we are testing since that can be done
at compile time which should help to improve performance on big-endian
systems.
CC: Alexander Duyck <alexander.h.duyck@redhat.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 7 +++
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 59 ++++++++++-------------
2 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
index ba96cb5..5f7d2f3 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
@@ -307,6 +307,13 @@ static inline bool ixgbevf_qv_disable(struct ixgbevf_q_vector *q_vector)
((_eitr) ? (1000000000 / ((_eitr) * 256)) : 8)
#define EITR_REG_TO_INTS_PER_SEC EITR_INTS_PER_SEC_TO_REG
+/* ixgbevf_test_staterr - tests bits in Rx descriptor status and error fields */
+static inline __le32 ixgbevf_test_staterr(union ixgbe_adv_rx_desc *rx_desc,
+ const u32 stat_err_bits)
+{
+ return rx_desc->wb.upper.status_error & cpu_to_le32(stat_err_bits);
+}
+
static inline u16 ixgbevf_desc_unused(struct ixgbevf_ring *ring)
{
u16 ntc = ring->next_to_clean;
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index deda74d..19062dc 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -331,15 +331,14 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector *q_vector,
* ixgbevf_receive_skb - Send a completed packet up the stack
* @q_vector: structure containing interrupt and ring information
* @skb: packet to send up
- * @status: hardware indication of status of receive
* @rx_desc: rx descriptor
**/
static void ixgbevf_receive_skb(struct ixgbevf_q_vector *q_vector,
- struct sk_buff *skb, u8 status,
+ struct sk_buff *skb,
union ixgbe_adv_rx_desc *rx_desc)
{
struct ixgbevf_adapter *adapter = q_vector->adapter;
- bool is_vlan = (status & IXGBE_RXD_STAT_VP);
+ bool is_vlan = !!ixgbevf_test_staterr(rx_desc, IXGBE_RXD_STAT_VP);
u16 tag = le16_to_cpu(rx_desc->wb.upper.vlan);
if (is_vlan && test_bit(tag & VLAN_VID_MASK, adapter->active_vlans))
@@ -355,11 +354,10 @@ static void ixgbevf_receive_skb(struct ixgbevf_q_vector *q_vector,
* ixgbevf_rx_skb - Helper function to determine proper Rx method
* @q_vector: structure containing interrupt and ring information
* @skb: packet to send up
- * @status: hardware indication of status of receive
* @rx_desc: rx descriptor
**/
static void ixgbevf_rx_skb(struct ixgbevf_q_vector *q_vector,
- struct sk_buff *skb, u8 status,
+ struct sk_buff *skb,
union ixgbe_adv_rx_desc *rx_desc)
{
#ifdef CONFIG_NET_RX_BUSY_POLL
@@ -372,17 +370,17 @@ static void ixgbevf_rx_skb(struct ixgbevf_q_vector *q_vector,
}
#endif /* CONFIG_NET_RX_BUSY_POLL */
- ixgbevf_receive_skb(q_vector, skb, status, rx_desc);
+ ixgbevf_receive_skb(q_vector, skb, rx_desc);
}
-/**
- * ixgbevf_rx_checksum - indicate in skb if hw indicated a good cksum
- * @ring: pointer to Rx descriptor ring structure
- * @status_err: hardware indication of status of receive
+/* ixgbevf_rx_checksum - indicate in skb if hw indicated a good cksum
+ * @ring: structure containig ring specific data
+ * @rx_desc: current Rx descriptor being processed
* @skb: skb currently being received and modified
- **/
+ */
static inline void ixgbevf_rx_checksum(struct ixgbevf_ring *ring,
- u32 status_err, struct sk_buff *skb)
+ union ixgbe_adv_rx_desc *rx_desc,
+ struct sk_buff *skb)
{
skb_checksum_none_assert(skb);
@@ -391,16 +389,16 @@ static inline void ixgbevf_rx_checksum(struct ixgbevf_ring *ring,
return;
/* if IP and error */
- if ((status_err & IXGBE_RXD_STAT_IPCS) &&
- (status_err & IXGBE_RXDADV_ERR_IPE)) {
+ if (ixgbevf_test_staterr(rx_desc, IXGBE_RXD_STAT_IPCS) &&
+ ixgbevf_test_staterr(rx_desc, IXGBE_RXDADV_ERR_IPE)) {
ring->rx_stats.csum_err++;
return;
}
- if (!(status_err & IXGBE_RXD_STAT_L4CS))
+ if (!ixgbevf_test_staterr(rx_desc, IXGBE_RXD_STAT_L4CS))
return;
- if (status_err & IXGBE_RXDADV_ERR_TCPE) {
+ if (ixgbevf_test_staterr(rx_desc, IXGBE_RXDADV_ERR_TCPE)) {
ring->rx_stats.csum_err++;
return;
}
@@ -520,33 +518,29 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
struct ixgbevf_rx_buffer *rx_buffer_info, *next_buffer;
struct sk_buff *skb;
unsigned int i;
- u32 len, staterr;
unsigned int total_rx_bytes = 0, total_rx_packets = 0;
u16 cleaned_count = ixgbevf_desc_unused(rx_ring);
i = rx_ring->next_to_clean;
rx_desc = IXGBEVF_RX_DESC(rx_ring, i);
- staterr = le32_to_cpu(rx_desc->wb.upper.status_error);
rx_buffer_info = &rx_ring->rx_buffer_info[i];
- while (staterr & IXGBE_RXD_STAT_DD) {
+ while (ixgbevf_test_staterr(rx_desc, IXGBE_RXD_STAT_DD)) {
if (!budget)
break;
budget--;
rmb(); /* read descriptor and rx_buffer_info after status DD */
- len = le16_to_cpu(rx_desc->wb.upper.length);
+
skb = rx_buffer_info->skb;
prefetch(skb->data - NET_IP_ALIGN);
rx_buffer_info->skb = NULL;
- if (rx_buffer_info->dma) {
- dma_unmap_single(rx_ring->dev, rx_buffer_info->dma,
- rx_ring->rx_buf_len,
- DMA_FROM_DEVICE);
- rx_buffer_info->dma = 0;
- skb_put(skb, len);
- }
+ dma_unmap_single(rx_ring->dev, rx_buffer_info->dma,
+ rx_ring->rx_buf_len,
+ DMA_FROM_DEVICE);
+ rx_buffer_info->dma = 0;
+ skb_put(skb, le16_to_cpu(rx_desc->wb.upper.length));
i++;
if (i == rx_ring->count)
@@ -558,7 +552,7 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
next_buffer = &rx_ring->rx_buffer_info[i];
- if (!(staterr & IXGBE_RXD_STAT_EOP)) {
+ if (!(ixgbevf_test_staterr(rx_desc, IXGBE_RXD_STAT_EOP))) {
skb->next = next_buffer->skb;
IXGBE_CB(skb->next)->prev = skb;
rx_ring->rx_stats.non_eop_descs++;
@@ -576,12 +570,13 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
}
/* ERR_MASK will only have valid bits if EOP set */
- if (unlikely(staterr & IXGBE_RXDADV_ERR_FRAME_ERR_MASK)) {
+ if (unlikely(ixgbevf_test_staterr(rx_desc,
+ IXGBE_RXDADV_ERR_FRAME_ERR_MASK))) {
dev_kfree_skb_irq(skb);
goto next_desc;
}
- ixgbevf_rx_checksum(rx_ring, staterr, skb);
+ ixgbevf_rx_checksum(rx_ring, rx_desc, skb);
/* probably a little skewed due to removing CRC */
total_rx_bytes += skb->len;
@@ -600,7 +595,7 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
goto next_desc;
}
- ixgbevf_rx_skb(q_vector, skb, staterr, rx_desc);
+ ixgbevf_rx_skb(q_vector, skb, rx_desc);
next_desc:
/* return some buffers to hardware, one at a time is too slow */
@@ -612,8 +607,6 @@ next_desc:
/* use prefetched values */
rx_desc = next_rxd;
rx_buffer_info = &rx_ring->rx_buffer_info[i];
-
- staterr = le32_to_cpu(rx_desc->wb.upper.status_error);
}
rx_ring->next_to_clean = i;
--
1.9.3
^ permalink raw reply related
* [net-next v2 01/15] ixgbevf: Update ixgbevf_alloc_rx_buffers to handle clearing of status bits
From: Jeff Kirsher @ 2014-11-20 23:08 UTC (permalink / raw)
To: davem
Cc: Emil Tantilov, netdev, nhorman, sassmann, jogreene,
Alexander Duyck, Jeff Kirsher
In-Reply-To: <1416524953-15161-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Emil Tantilov <emil.s.tantilov@intel.com>
Instead of clearing the status bits in the cleanup it makes more sense to
just clear the status bits on allocation. This way we can leave the Rx
descriptor rings as a read only memory block until we actually have buffers
to give back to the hardware.
CC: Alexander Duyck <alexander.h.duyck@redhat.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 132 +++++++++++++---------
1 file changed, 80 insertions(+), 52 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 030a219..deda74d 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -143,21 +143,6 @@ u32 ixgbevf_read_reg(struct ixgbe_hw *hw, u32 reg)
return value;
}
-static inline void ixgbevf_release_rx_desc(struct ixgbevf_ring *rx_ring,
- u32 val)
-{
- rx_ring->next_to_use = val;
-
- /*
- * Force memory writes to complete before letting h/w
- * know there are new descriptors to fetch. (Only
- * applicable for weak-ordered memory model archs,
- * such as IA-64).
- */
- wmb();
- ixgbevf_write_tail(rx_ring, val);
-}
-
/**
* ixgbevf_set_ivar - set IVAR registers - maps interrupt causes to vectors
* @adapter: pointer to adapter struct
@@ -424,52 +409,99 @@ static inline void ixgbevf_rx_checksum(struct ixgbevf_ring *ring,
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
+static bool ixgbevf_alloc_mapped_skb(struct ixgbevf_ring *rx_ring,
+ struct ixgbevf_rx_buffer *bi)
+{
+ struct sk_buff *skb = bi->skb;
+ dma_addr_t dma = bi->dma;
+
+ if (unlikely(skb))
+ return true;
+
+ skb = netdev_alloc_skb_ip_align(rx_ring->netdev,
+ rx_ring->rx_buf_len);
+ if (unlikely(!skb)) {
+ rx_ring->rx_stats.alloc_rx_buff_failed++;
+ return false;
+ }
+
+ dma = dma_map_single(rx_ring->dev, skb->data,
+ rx_ring->rx_buf_len, DMA_FROM_DEVICE);
+
+ /* if mapping failed free memory back to system since
+ * there isn't much point in holding memory we can't use
+ */
+ if (dma_mapping_error(rx_ring->dev, dma)) {
+ dev_kfree_skb_any(skb);
+
+ rx_ring->rx_stats.alloc_rx_buff_failed++;
+ return false;
+ }
+
+ bi->skb = skb;
+ bi->dma = dma;
+
+ return true;
+}
+
/**
* ixgbevf_alloc_rx_buffers - Replace used receive buffers; packet split
* @rx_ring: rx descriptor ring (for a specific queue) to setup buffers on
+ * @cleaned_count: number of buffers to replace
**/
static void ixgbevf_alloc_rx_buffers(struct ixgbevf_ring *rx_ring,
- int cleaned_count)
+ u16 cleaned_count)
{
union ixgbe_adv_rx_desc *rx_desc;
struct ixgbevf_rx_buffer *bi;
unsigned int i = rx_ring->next_to_use;
- while (cleaned_count--) {
- rx_desc = IXGBEVF_RX_DESC(rx_ring, i);
- bi = &rx_ring->rx_buffer_info[i];
-
- if (!bi->skb) {
- struct sk_buff *skb;
+ /* nothing to do or no valid netdev defined */
+ if (!cleaned_count || !rx_ring->netdev)
+ return;
- skb = netdev_alloc_skb_ip_align(rx_ring->netdev,
- rx_ring->rx_buf_len);
- if (!skb)
- goto no_buffers;
+ rx_desc = IXGBEVF_RX_DESC(rx_ring, i);
+ bi = &rx_ring->rx_buffer_info[i];
+ i -= rx_ring->count;
- bi->skb = skb;
+ do {
+ if (!ixgbevf_alloc_mapped_skb(rx_ring, bi))
+ break;
- bi->dma = dma_map_single(rx_ring->dev, skb->data,
- rx_ring->rx_buf_len,
- DMA_FROM_DEVICE);
- if (dma_mapping_error(rx_ring->dev, bi->dma)) {
- dev_kfree_skb(skb);
- bi->skb = NULL;
- dev_err(rx_ring->dev, "Rx DMA map failed\n");
- break;
- }
- }
+ /* Refresh the desc even if pkt_addr didn't change
+ * because each write-back erases this info.
+ */
rx_desc->read.pkt_addr = cpu_to_le64(bi->dma);
+ rx_desc++;
+ bi++;
i++;
- if (i == rx_ring->count)
- i = 0;
- }
+ if (unlikely(!i)) {
+ rx_desc = IXGBEVF_RX_DESC(rx_ring, 0);
+ bi = rx_ring->rx_buffer_info;
+ i -= rx_ring->count;
+ }
+
+ /* clear the hdr_addr for the next_to_use descriptor */
+ rx_desc->read.hdr_addr = 0;
+
+ cleaned_count--;
+ } while (cleaned_count);
-no_buffers:
- rx_ring->rx_stats.alloc_rx_buff_failed++;
- if (rx_ring->next_to_use != i)
- ixgbevf_release_rx_desc(rx_ring, i);
+ i += rx_ring->count;
+
+ if (rx_ring->next_to_use != i) {
+ /* record the next descriptor to use */
+ rx_ring->next_to_use = i;
+
+ /* Force memory writes to complete before letting h/w
+ * know there are new descriptors to fetch. (Only
+ * applicable for weak-ordered memory model archs,
+ * such as IA-64).
+ */
+ wmb();
+ ixgbevf_write_tail(rx_ring, i);
+ }
}
static inline void ixgbevf_irq_enable_queues(struct ixgbevf_adapter *adapter,
@@ -489,8 +521,8 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
struct sk_buff *skb;
unsigned int i;
u32 len, staterr;
- int cleaned_count = 0;
unsigned int total_rx_bytes = 0, total_rx_packets = 0;
+ u16 cleaned_count = ixgbevf_desc_unused(rx_ring);
i = rx_ring->next_to_clean;
rx_desc = IXGBEVF_RX_DESC(rx_ring, i);
@@ -571,8 +603,6 @@ static int ixgbevf_clean_rx_irq(struct ixgbevf_q_vector *q_vector,
ixgbevf_rx_skb(q_vector, skb, staterr, rx_desc);
next_desc:
- rx_desc->wb.upper.status_error = 0;
-
/* return some buffers to hardware, one at a time is too slow */
if (cleaned_count >= IXGBEVF_RX_BUFFER_WRITE) {
ixgbevf_alloc_rx_buffers(rx_ring, cleaned_count);
@@ -587,11 +617,6 @@ next_desc:
}
rx_ring->next_to_clean = i;
- cleaned_count = ixgbevf_desc_unused(rx_ring);
-
- if (cleaned_count)
- ixgbevf_alloc_rx_buffers(rx_ring, cleaned_count);
-
u64_stats_update_begin(&rx_ring->syncp);
rx_ring->stats.packets += total_rx_packets;
rx_ring->stats.bytes += total_rx_bytes;
@@ -599,6 +624,9 @@ next_desc:
q_vector->rx.total_packets += total_rx_packets;
q_vector->rx.total_bytes += total_rx_bytes;
+ if (cleaned_count)
+ ixgbevf_alloc_rx_buffers(rx_ring, cleaned_count);
+
return total_rx_packets;
}
--
1.9.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