* [PATCH] NFC: hci/core.c: local variables should not be exposed globally
From: H Hartley Sweeten @ 2012-04-26 18:05 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
The variable 'hci_nfc_ops' is only referenced in this file and
should be marked static to prevent it from being exposed globally.
Quites the sparse warning:
warning: symbol 'hci_nfc_ops' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/hci/core.c b/net/nfc/hci/core.c
index 86fd00d..0ce5ee4 100644
--- a/net/nfc/hci/core.c
+++ b/net/nfc/hci/core.c
@@ -605,7 +605,7 @@ static int hci_data_exchange(struct nfc_dev *nfc_dev, u32 target_idx,
return 0;
}
-struct nfc_ops hci_nfc_ops = {
+static struct nfc_ops hci_nfc_ops = {
.dev_up = hci_dev_up,
.dev_down = hci_dev_down,
.start_poll = hci_start_poll,
^ permalink raw reply related
* [PATCH] NFC: nci/ntf.c: quiet sparse noise about plain integer as NULL pointer
From: H Hartley Sweeten @ 2012-04-26 17:59 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
Pointers should be cleared with NULL, not 0.
Quiets a couple sparse warnings of the type:
warning: Using plain integer as NULL pointer
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
index 99e1632..cb26461 100644
--- a/net/nfc/nci/ntf.c
+++ b/net/nfc/nci/ntf.c
@@ -497,7 +497,7 @@ static void nci_rf_deactivate_ntf_packet(struct nci_dev *ndev,
/* drop partial rx data packet */
if (ndev->rx_data_reassembly) {
kfree_skb(ndev->rx_data_reassembly);
- ndev->rx_data_reassembly = 0;
+ ndev->rx_data_reassembly = NULL;
}
/* complete the data exchange transaction, if exists */
^ permalink raw reply related
* [PATCH] NFC: nci/lib.c: include header for exported symbol prototype
From: H Hartley Sweeten @ 2012-04-26 17:56 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
Include the header to pickup the exported symbol prototype.
Quites the sparse warning:
warning: symbol 'nci_to_errno' was not declared. Should it be static?
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
CC: Samuel Ortiz <sameo@linux.intel.com>
CC: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/nci/lib.c b/net/nfc/nci/lib.c
index 6a63e5e..6b7fd26 100644
--- a/net/nfc/nci/lib.c
+++ b/net/nfc/nci/lib.c
@@ -31,6 +31,7 @@
#include <linux/errno.h>
#include <net/nfc/nci.h>
+#include <net/nfc/nci_core.h>
/* NCI status codes to Unix errno mapping */
int nci_to_errno(__u8 code)
^ permalink raw reply related
* [PATCH] NFC: nci/data.c: quiet sparse noise about plain integer as NULL pointer
From: H Hartley Sweeten @ 2012-04-26 17:31 UTC (permalink / raw)
To: Linux Kernel
Cc: netdev, linux-wireless, lauro.venancio, aloisio.almeida, sameo,
davem
Pointers should be cleared with NULL, not 0.
Quiets a couple sparse warnings of the type:
warning: Using plain integer as NULL pointer
Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Lauro Ramos Venancio <lauro.venancio@openbossa.org>
Cc: Aloisio Almeida Jr <aloisio.almeida@openbossa.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
index a0bc326..76c48c5 100644
--- a/net/nfc/nci/data.c
+++ b/net/nfc/nci/data.c
@@ -49,7 +49,7 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
if (cb) {
ndev->data_exchange_cb = NULL;
- ndev->data_exchange_cb_context = 0;
+ ndev->data_exchange_cb_context = NULL;
/* forward skb to nfc core */
cb(cb_context, skb, err);
@@ -200,10 +200,10 @@ static void nci_add_rx_data_frag(struct nci_dev *ndev,
pr_err("error adding room for accumulated rx data\n");
kfree_skb(skb);
- skb = 0;
+ skb = NULL;
kfree_skb(ndev->rx_data_reassembly);
- ndev->rx_data_reassembly = 0;
+ ndev->rx_data_reassembly = NULL;
err = -ENOMEM;
goto exit;
@@ -216,7 +216,7 @@ static void nci_add_rx_data_frag(struct nci_dev *ndev,
/* third, free old reassembly */
kfree_skb(ndev->rx_data_reassembly);
- ndev->rx_data_reassembly = 0;
+ ndev->rx_data_reassembly = NULL;
}
if (pbf == NCI_PBF_CONT) {
^ permalink raw reply related
* Re: [PATCH net-next 2/5] be2net: Fix to allow setting of debug levels in the firmware.
From: Ben Hutchings @ 2012-04-26 17:29 UTC (permalink / raw)
To: Somnath Kotur; +Cc: netdev
In-Reply-To: <464dcb28-2450-4e0b-a30f-d8c9104bac86@exht1.ad.emulex.com>
On Thu, 2012-04-26 at 19:57 +0530, Somnath Kotur wrote:
> From: Somnath Kotur <somnath.kotur@emulex.com>
[...]
> @@ -893,6 +959,7 @@ const struct ethtool_ops be_ethtool_ops = {
> .set_pauseparam = be_set_pauseparam,
> .get_strings = be_get_stat_strings,
> .set_phys_id = be_set_phys_id,
> + .set_msglevel = be_set_msglevel,
> .get_sset_count = be_get_sset_count,
> .get_ethtool_stats = be_get_ethtool_stats,
> .get_regs_len = be_get_reg_len,
This operation is intended for controlling logging by the driver, and
the flags are defined in <linux/netdevice.h>. (Not exported to userland
yet, but ethtool knows their names.)
If your firmware supports some kind of logging then it may be reasonable
to have this control both driver and firmware, but not *just* the
firmware.
You should also implement the get_msglevel operation at the same time.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH net-next 3/5] be2net: Fix to apply duplex value as unknown when link is down.
From: Ben Hutchings @ 2012-04-26 17:24 UTC (permalink / raw)
To: Somnath Kotur; +Cc: netdev
In-Reply-To: <cf78444f-636f-44c5-beb7-5626b8a3df17@exht1.ad.emulex.com>
On Thu, 2012-04-26 at 19:58 +0530, Somnath Kotur wrote:
> From: Somnath Kotur <somnath.kotur@emulex.com>
>
>
> Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
> ---
> drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c
> index 5296df5..bb2add7 100644
> --- a/drivers/net/ethernet/emulex/benet/be_ethtool.c
> +++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c
> @@ -618,7 +618,7 @@ static int be_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
> ecmd->supported = adapter->phy.supported;
> }
>
> - ecmd->duplex = DUPLEX_FULL;
> + ecmd->duplex = netif_carrier_ok(netdev) ? DUPLEX_FULL : -1;
> ecmd->phy_address = adapter->port_num;
>
> return 0;
I don't see any problem with reporting full-duplex all the time if you
don't support any half-duplex link modes. But we don't yet have
consistency between drivers in speed/duplex reporting while the link is
down, so I won't insist that that is the right thing to do.
However you should use DUPLEX_UNKNOWN rather than -1.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [net-next 1/4 (V3)] net: ethtool: add the EEE support
From: Ben Hutchings @ 2012-04-26 17:17 UTC (permalink / raw)
To: Giuseppe CAVALLARO; +Cc: netdev, rayagond, davem
In-Reply-To: <4F98FDCC.3040807@st.com>
On Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote:
> Hello Ben
>
> On 4/19/2012 5:30 PM, Ben Hutchings wrote:
> [snip]
> >> I'm changing the code for getting/setting the EEE capability and trying
> >> to follow your suggestions.
> >>
> >> The "get" will show the following things; this is a bit different of the
> >> points "a" "b" and "c" we had discussed. Maybe, this could also be a
> >> more complete (*) .
> >> The ethtool (see output below as example) could report the phy
> >> (supported/advertised/lp_advertised) and mac eee capabilities separately.
> >
> > Sounds reasonable.
> >
> >> The "set" will be useful for some eth devices (like the stmmac) that can
> >> stop/enable internally the eee capability (at mac level).
> >
> > I don't know much about EEE, but shouldn't the driver take care of
> > configuring the MAC for this whenever the PHY is set to advertise EEE
> > capability?
>
> Yes indeed this can be done at driver level. So could I definitely
> remove it from ethtool? What do you suggest?
>
> In case of the stmmac I could add a specific driver option via sys to
> enable/disable the eee and set timer.
Generally, ethtool doesn't distinguish MAC and PHY settings because they
have to be configured consistently for the device to do anything useful.
If there is some good use for enabling EEE in the MAC and not the PHY,
or vice versa, then this should be exposed in the ethtool interface.
But if not then I don't believe it needs to be in either an ethtool or a
driver-specific interface.
> >> [snip]
> >>
> >> Current message level: 0x0000003f (63)
> >> drv probe link timer ifdown ifup
> >> Link detected: yes
> >> Energy-Efficient Ethernet: -------------------------
> >> MAC supports: yes |-> related to MAC side |
> >> phy supports modes: ... |-> from MMD 3.20 |
> >> phy advertising modes: ... |-> from MMD 7.60 |
> >> LP advertising modes: ... |-> from MMD 7.61 |
> >> --------------------------
> >> (*)
> >> PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc
> >> for each advertising modes and also for phy support (reg 3.20).
> >
> > Yes, that's the sort of information I would expect to see (but try to be
> > consistent with the wording used for AN).:
>
> e.g. SUPPORTED_100baseT_EEE ... ADVERTISED_<...>
I meant the wording used in the ethtool output: 'supported',
'advertised', 'link partner advertised' rather than 'phy supports',
'phy advertising', 'LP advertising'.
> > The EEE advertising mask presumably should be changeable similarly to
> > the AN advertising mask ('ethtool -s <devname> eeeadv <mask>'). But I
> > don't know how the two advertising masks interact. Is one supposed to
> > be a subset of the other? Currently ethtool automatically changes the
> > AN advertising mask in response to a speed/duplex change; should it also
> > try to change the EEE advertising mask?
>
> I've just verified the IEEE (Table 45–150a—EEE advertisement register
> (Register 7.60) bit definitions) and sorry for my delay in reply but I
> was in trouble because looking at the registers for the phy (I am using)
> the reg 7.60 was in RO and I couldn't understand how to set the mask.
> I confirm that the Adv reg from the std is R/W and the mask as you
> suggest could be set according to the speed.
> The EEE should work on duplex mode only.
>
> I wonder so if if the final patch I should have no new option for the
> ethtool command and EEE info are directly passed from the kernel like
> speed and duplex when call get_settings.
Are you suggesting to define EEE mode flags in the existing supported,
advertising and lp_advertising masks?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] b44: properly use pr_fmt()
From: Luis R. Rodriguez @ 2012-04-26 15:53 UTC (permalink / raw)
To: David Miller
Cc: zambrano, netdev, linux-wireless, linux-bluetooth,
Arend Van Spriel, Joe Perches
In-Reply-To: <CAB=NE6ViDGHPn=OsqFK1rJb-5J+1xzvf3h9+Rd-MgmWpn-T2fA@mail.gmail.com>
On Thu, Apr 26, 2012 at 6:02 AM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Thu, Apr 26, 2012 at 5:23 AM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
>> On Mon, Apr 23, 2012 at 9:47 PM, David Miller <davem@davemloft.net> wrote:
>>> From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
>>> Date: Mon, 23 Apr 2012 19:46:43 -0700
>>>
>>>> From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>
>>>>
>>>> pr_fmt() is either defined or we redefine it. Typically
>>>> drivers define it prior to including printk.h but this
>>>> is done under the assumption that no other subsystem
>>>> it uses has already defined pr_fmt(). In such cases
>>>> pr_fmt() should be undefined and redefined.
>>>>
>>>> Doing this properly shaves down compilation time quite
>>>> considerably.
>>>>
>>>> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
>>>
>>> Every driver defines pr_fmt then includes the headers. I do not
>>> see any other place which performs the initial undef of pr_fmt
>>> like you want to start doing here.
>>
>> One example is, and that is where I got the hint:
>>
>> drivers/net/wireless/mwifiex/decl.h
>>
>>> If there is a reason we should start doing this, it would appear
>>> to be a universal reason, not one specific to this driver.
>>
>> Sure.
>>
>>> If that's the case, we should formally agree to this and then work
>>> on gradually converting all the drivers in reasonably sized chunks.
>>
>> Understood.
>>
>>> Either way I can't apply this patch as-is, sorry Luis.
>>
>> So the issue can occur if any parent header ends up defining this,
>> this could happen if a subsystem wants to define the pr_fmt in their
>> own header and a driver for example wants to override this. In theory
>> this does not occur but in practice this may vary and if we want to
>> ensure this does not happen perhaps the thing to do is to document
>> this as such.
>>
>> I just tested allmodconfig though and checked compilation for V=1
>> against drivers/net/wireless/ and found no issues against linux-next
>> though so in practice at least for wireless networking it seems we are
>> squeaky clean in its usage. I'll test across the kernel though next to
>> see if there are any real violators of the assumption. Unfortunately
>> I've hit an issue with compiling allmodconfig already with linux-next
>> that is already reported so my homework will be to find a shiny
>> linux-next to compile test against allmodconfig.
>>
>> An area that this does clearly happen but that likely does not merit
>> and upstream change for is in the backport work compat that defines
>> this for older kernels and later we have to also backport users of
>> pr_fmt such as pr_emerg_once. Ultimately drivers will want to actually
>> then override this and in that case the undef is necessary as
>> otherwise you get the redefinition warning which incurs in practice a
>> considerable CPU performance hit at compilation time.
>
> Additionally Arend has pointed pointed out that previously printk.h
> was removed as a direct include as well given that kernel.h includes
> it already:
>
> https://lkml.org/lkml/2011/5/25/331
>
> So this would be a second issue with this patch.
OK I found no pr_fmt warnings at all for make allmodconfig ; make V=1
; on v3.4-rc4. For this and others reason stated then yes, this patch
is simply bogus.
Luis
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 16:31 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <20120426162819.GD2479@BohrerMBP.rgmadvisors.com>
On Thu, 2012-04-26 at 11:28 -0500, Shawn Bohrer wrote:
> No in this case it is 300 unique multicast addresses, and there is one
> socket listening to each multicast address. So a single message is
> only copied once to a single socket. The bottle neck appears to be
> that even though a single message is only going to get copied to a
> single socket we still have to walk the list of all 300 sockets while
> holding the spin lock to figure that out. The incoming packet rate is
> also roughly evenly distributed across all 300 multicast addresses so
> even though we have multiple receive queues they are all contending
> for the same spin lock.
>
I repeat my question : Are these 300 sockets bound to the same UDP
port ?
If not, they should be spreaded in hash table.
You can make this hash table very big to reduce hash collisions
Boot parameter : uhash_entries=65536
^ permalink raw reply
* Re: use-after-free in usbnet
From: Huajun Li @ 2012-04-26 16:30 UTC (permalink / raw)
To: Ming Lei
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CACVXFVOFGA-Rn3X2HGGM-AYSygiysNnRo0RiRzmJhZ=swtZz_g@mail.gmail.com>
On Thu, Apr 26, 2012 at 1:02 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> Hi Huajun,
>
> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>
>>
>> Above patch has already been integrated to mainline. However, maybe
>> there still exists another potentail use-after-free issue, here is a
>> case:
>> After release the lock in unlink_urbs(), defer_bh() may move
>> current skb from rxq/txq to dev->done queue, even cause the skb be
>> released. Then in next loop cycle, it can't refer to expected skb, and
>> may Oops again.
>>
>> To easily reproduce it, in unlink_urbs(), you can delay a short time
>> after usb_put_urb(urb), then disconnect your device while transferring
>> data, and repeat it times you will find errors on your screen.
>>
>> Following is a draft patch to guarantee the queue consistent, and
>> refer to expected skb in each loop cycle:
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index b7b3f5b..6da0141 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -578,16 +578,28 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
>> static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
>> {
>> unsigned long flags;
>> - struct sk_buff *skb, *skbnext;
>> + struct sk_buff *skb;
>> int count = 0;
>>
>> spin_lock_irqsave (&q->lock, flags);
>> - skb_queue_walk_safe(q, skb, skbnext) {
>> + while (!skb_queue_empty(q)) {
>> struct skb_data *entry;
>> struct urb *urb;
>> int retval;
>>
>> - entry = (struct skb_data *) skb->cb;
>> + skb_queue_walk(q, skb) {
>> + entry = (struct skb_data *)skb->cb;
>> + if (entry->state == rx_done ||
>> + entry->state == tx_done ||
>> + entry->state == rx_cleanup)
>> + continue;
>> + else
>> + break;
>> + }
>> +
>> + if (skb == (struct sk_buff *)(q))
>> + break;
>> +
>> urb = entry->urb;
>>
>>
>
> After thinking about the issue further, the basic idea of your patch is good,
> but not safe(double unlink, also races on accessing entry->state), so I write
> a new one based on your patch.
>
> Are you OK this new one?
>
Yes, no more issue that I can see by now, thanks.
> Thanks,
> --
> Ming Lei
>
> From 89eb41968caa9523c90c1cf7b7e2259db00e04b8 Mon Sep 17 00:00:00 2001
> From: Ming Lei <tom.leiming@gmail.com>
> Date: Thu, 26 Apr 2012 11:33:46 +0800
> Subject: [PATCH] usbnet: fix skb traversing races during unlink
>
> Commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d(net/usbnet: avoid
> recursive locking in usbnet_stop()) fixes the recursive locking
> problem by releasing the skb queue lock before unlink, but may
> cause skb traversing races:
> - after URB is unlinked and the queue lock is released,
> the refered skb and skb->next may be moved to done queue,
> even be released
> - in skb_queue_walk_safe, the next skb is still obtained
> by next pointer of the last skb
> - so maybe trigger oops or other problems
>
> This patch extends the usage of entry->state to describe 'start_unlink'
> state, so always holding the queue(rx/tx) lock to change the state if
> the referd skb is in rx or tx queue because we need to know if the
> refered urb has been started unlinking in unlink_urbs.
>
> Also the patch uses usb_block_urb introduced by Oliver to block
> URB resubmit in complete handler if the URB will be unlinked.
>
> The other part of this patch is based on Hugjun's patch:
s/Hugjun/Huajun
> always traverse from head of the tx/rx queue to get skb which is
> to be unlinked but not been started unlinking.
> ---
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Shawn Bohrer @ 2012-04-26 16:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1335455595.2775.47.camel@edumazet-glaptop>
On Thu, Apr 26, 2012 at 05:53:15PM +0200, Eric Dumazet wrote:
> On Thu, 2012-04-26 at 10:15 -0500, Shawn Bohrer wrote:
> > I've been doing some UDP multicast benchmarking and noticed that as we
> > increase the number of sockets/multicast addresses the performance
> > degrades. The test I'm running has multiple machines sending packets
> > on multiple multicast addresses. A single receiving machine opens one
> > socket per multicast address to receive all the packets. The
> > receiving process is bound to a core that is not processing
> > interrupts.
> >
> > Running this test with 300 multicast addresses and sockets and
> > profiling the receiving machine with 'perf -a -g' I can see the
> > following:
> >
> >
> > # Events: 45K cycles
> > #
> > # Overhead
> > # ........ .....................................
> > #
> > 52.56% [k] _raw_spin_lock
> > |
> > |--99.09%-- __udp4_lib_mcast_deliver
> > 20.10% [k] __udp4_lib_mcast_deliver
> > |
> > --- __udp4_lib_rcv
> >
> > So if I understand this correctly 52.56% of the time is spent
> > contending for the spin_lock in __udp4_lib_mcast_deliver. If I
> > understand the code correctly it appears that for every packet
> > received we walk the list of all UDP sockets while holding the
> > spin_lock. Therefore I believe the thing that hurts so much in this
> > case is that we have a lot of UDP sockets.
> >
> > Are there any ideas on how we can improve the performance in this
> > case? Honestly I have two ideas though my understanding of the
> > network stack is limited and it is unclear to me how to implement
> > either of them.
> >
> > The first idea is to use RCU instead of acquiring the spin_lock. This
> > is what the Unicast path does however looking back to 271b72c7 "udp:
> > RCU handling for Unicast packets." Eric points out that the multicast
> > path is difficult. It appears from that commit description that the
> > problem is that since we have to find all sockets interested in
> > receiving the packet instead of just one that restarting the scan of
> > the hlist could lead us to deliver the packet twice to the same
> > socket. That commit is rather old though I believe things may have
> > changed. Looking at commit 1240d137 "ipv4: udp: Optimise multicast
> > reception" I can see that Eric also has already done some work to
> > reduce how long the spin_lock is held in __udp4_lib_mcast_deliver().
> > That commit also says "It's also a base for a future RCU conversion of
> > multicast recption". Is the idea that you could remove duplicate
> > sockets within flush_stack()? Actually I don't think that would work
> > since flush_stack() can be called multiple times if the stack gets
> > full.
> >
> > The second idea would be to hash the sockets to reduce the number of
> > sockets to walk for each packet. Once again it looks like the Unicast
> > path already does this in commits 512615b6b "udp: secondary hash on
> > (local port, local address)" and 5051ebd27 "ipv4: udp: optimize
> > unicast RX path". Perhaps these hash lists could be used, however I
> > don't think they can since they currently use RCU and thus it might
> > depend on converting to RCU first.
>
> Let me understand
>
> You have 300 sockets bound to the same port, so a single message must be
> copied 300 times and delivered to those sockets ?
No in this case it is 300 unique multicast addresses, and there is one
socket listening to each multicast address. So a single message is
only copied once to a single socket. The bottle neck appears to be
that even though a single message is only going to get copied to a
single socket we still have to walk the list of all 300 sockets while
holding the spin lock to figure that out. The incoming packet rate is
also roughly evenly distributed across all 300 multicast addresses so
even though we have multiple receive queues they are all contending
for the same spin lock.
--
Shawn
--
---------------------------------------------------------------
This email, along with any attachments, is confidential. If you
believe you received this message in error, please contact the
sender immediately and delete all copies of the message.
Thank you.
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 16:21 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <1335457112.2775.50.camel@edumazet-glaptop>
On Thu, 2012-04-26 at 18:18 +0200, Eric Dumazet wrote:
> On Thu, 2012-04-26 at 17:53 +0200, Eric Dumazet wrote:
>
> > Let me understand
> >
> > You have 300 sockets bound to the same port, so a single message must be
> > copied 300 times and delivered to those sockets ?
> >
> >
>
> Please try the following patch. It should allow up to 512 sockets (on
> x86_64) to be stored in stack, and delivery performed out of the locked
> section.
>
> net/ipv4/udp.c | 16 ++++++++++++----
> net/ipv6/udp.c | 15 +++++++++++----
> 2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 279fd08..beb9ea6 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1539,13 +1539,20 @@ static void flush_stack(struct sock **stack, unsigned int count,
> static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> struct udphdr *uh,
> __be32 saddr, __be32 daddr,
> - struct udp_table *udptable)
> + struct udp_table *udptable,
> + int proto)
> {
> - struct sock *sk, *stack[256 / sizeof(struct sock *)];
> + struct sock *sk, **stack;
> struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
> int dif;
> unsigned int i, count = 0;
>
> + stack = kmalloc(PAGE_SIZE, GFP_ATOMIC);
> + if (unlikely(!stack)) {
> + UDP_INC_STATS_BH(net, UDP_MIB_RCVBUFERRORS, proto == IPPROTO_UDPLITE);
> + kfree_skb(skb);
> + return 0;
> + }
> spin_lock(&hslot->lock);
> sk = sk_nulls_head(&hslot->head);
> dif = skb->dev->ifindex;
> @@ -1554,7 +1561,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> stack[count++] = sk;
> sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
> daddr, uh->source, saddr, dif);
> - if (unlikely(count == ARRAY_SIZE(stack))) {
> + if (unlikely(count == PAGE_SIZE/sizeof(*sk))) {
Oops, should be PAGE_SIZE/sizeof(sk) of course
(same problem in ipv6/udp.c)
> if (!sk)
> break;
> flush_stack(stack, count, skb, ~0);
> @@ -1580,6 +1587,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
> } else {
> kfree_skb(skb);
> }
> + kfree(stack);
> return 0;
> }
>
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 16:18 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <1335455595.2775.47.camel@edumazet-glaptop>
On Thu, 2012-04-26 at 17:53 +0200, Eric Dumazet wrote:
> Let me understand
>
> You have 300 sockets bound to the same port, so a single message must be
> copied 300 times and delivered to those sockets ?
>
>
Please try the following patch. It should allow up to 512 sockets (on
x86_64) to be stored in stack, and delivery performed out of the locked
section.
net/ipv4/udp.c | 16 ++++++++++++----
net/ipv6/udp.c | 15 +++++++++++----
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 279fd08..beb9ea6 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1539,13 +1539,20 @@ static void flush_stack(struct sock **stack, unsigned int count,
static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
struct udphdr *uh,
__be32 saddr, __be32 daddr,
- struct udp_table *udptable)
+ struct udp_table *udptable,
+ int proto)
{
- struct sock *sk, *stack[256 / sizeof(struct sock *)];
+ struct sock *sk, **stack;
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
unsigned int i, count = 0;
+ stack = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+ if (unlikely(!stack)) {
+ UDP_INC_STATS_BH(net, UDP_MIB_RCVBUFERRORS, proto == IPPROTO_UDPLITE);
+ kfree_skb(skb);
+ return 0;
+ }
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = skb->dev->ifindex;
@@ -1554,7 +1561,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
stack[count++] = sk;
sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
daddr, uh->source, saddr, dif);
- if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (unlikely(count == PAGE_SIZE/sizeof(*sk))) {
if (!sk)
break;
flush_stack(stack, count, skb, ~0);
@@ -1580,6 +1587,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
} else {
kfree_skb(skb);
}
+ kfree(stack);
return 0;
}
@@ -1661,7 +1669,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
return __udp4_lib_mcast_deliver(net, skb, uh,
- saddr, daddr, udptable);
+ saddr, daddr, udptable, proto);
sk = __udp4_lib_lookup_skb(skb, uh->source, uh->dest, udptable);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index d39bbc9..fc79b87 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -640,14 +640,20 @@ drop:
*/
static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
const struct in6_addr *saddr, const struct in6_addr *daddr,
- struct udp_table *udptable)
+ struct udp_table *udptable, int proto)
{
- struct sock *sk, *stack[256 / sizeof(struct sock *)];
+ struct sock *sk, **stack;
const struct udphdr *uh = udp_hdr(skb);
struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
int dif;
unsigned int i, count = 0;
+ stack = kmalloc(PAGE_SIZE, GFP_ATOMIC);
+ if (unlikely(!stack)) {
+ UDP6_INC_STATS_BH(net, UDP_MIB_RCVBUFERRORS, proto == IPPROTO_UDPLITE);
+ kfree_skb(skb);
+ return 0;
+ }
spin_lock(&hslot->lock);
sk = sk_nulls_head(&hslot->head);
dif = inet6_iif(skb);
@@ -656,7 +662,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
stack[count++] = sk;
sk = udp_v6_mcast_next(net, sk_nulls_next(sk), uh->dest, daddr,
uh->source, saddr, dif);
- if (unlikely(count == ARRAY_SIZE(stack))) {
+ if (unlikely(count == PAGE_SIZE/sizeof(*sk))) {
if (!sk)
break;
flush_stack(stack, count, skb, ~0);
@@ -679,6 +685,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
} else {
kfree_skb(skb);
}
+ kfree(stack);
return 0;
}
@@ -763,7 +770,7 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
*/
if (ipv6_addr_is_multicast(daddr))
return __udp6_lib_mcast_deliver(net, skb,
- saddr, daddr, udptable);
+ saddr, daddr, udptable, proto);
/* Unicast */
^ permalink raw reply related
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Kyle Mestery (kmestery) @ 2012-04-26 16:16 UTC (permalink / raw)
To: Jesse Gross
Cc: <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
<eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
<netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
<jhs-jkUAjuhPggJWk0Htik3J/w@public.gmane.org>,
<shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org>,
David Miller
In-Reply-To: <CAEP_g=8VQizt5iUc_yR+PynMYpZgD4ep+o379JK8k-KCKMYgmg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Apr 26, 2012, at 11:13 AM, Jesse Gross wrote:
> On Thu, Apr 26, 2012 at 12:13 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>> On Wed, Apr 25, 2012 at 10:17:25AM -0700, Jesse Gross wrote:
>>> On Wed, Apr 25, 2012 at 1:39 AM, Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org> wrote:
>>>>
>>>> Hi Kyle,
>>>>
>>>> the component that is of most interest to me is enabling OVS to use in-tree
>>>> tunnelling code - as it seems that makes most sense for an implementation
>>>> of STT. I have taken a brief look over your vxlan work and it isn't clear
>>>> to me if it is moving towards being an in-tree implementation. Moreover,
>>>> I'm a rather unclear on what changes need to be made to OVS in order for
>>>> in-tree tunneling to be used.
>>>>
>>>> My recollection is that OVS did make use of in-tree tunnelling code
>>>> but this was removed in favour of the current implementation for various
>>>> reasons (performance being one IIRC). I gather that revisiting in-tree
>>>> tunnelling won't revisit the previous set of problems. But I'm unclear how.
>>>>
>>>> Jesse, is it possible for you to describe that in a little detail
>>>> or point me to some information?
>>>
>>> This was what I had originally written a while back, although it's
>>> more about OVS internally and less about how to connect to the in-tree
>>> code:
>>> http://openvswitch.org/pipermail/dev/2012-February/014779.html
>>>
>>> In order to flexibly implement support for current and future tunnel
>>> protocols OVS needs to be able to get/set information about the outer
>>> tunnel header when processing the inner packet. At the very least
>>> this is src/dst IP addresses and the key/ID/VNI/etc. In the upstream
>>> tunnel implementations those are implicitly encoded in the device that
>>> sends or receives the packet. However, this has a two problems:
>>> number of devices and ability to handle unknown values. We addressed
>>> part of this problem by allowing the tunnel ID to be set and matched
>>> through the OVS flow table and an action. In order to do this with
>>> the in-tree tunneling code, we obviously need a way of passing this
>>> information around since it would currently get lost as we pass
>>> through the Linux device layer.
>>>
>>> The plan to deal with that is to add a function to the in-tree
>>> tunneling code that allows a skb to be encapsulated with specific
>>> parameters and conversely a hook to receive decapsulated packets along
>>> with header info. This would make all of the kernel tunneling code
>>> common, while still giving OVS userspace the ability to implement
>>> essentially any type of tunneling policy. In many ways, this is very
>>> similar to how vlans look in OVS today.
>>>
>>> While it would be possible to implement the hook to use the in-tree
>>> tunnel code today without a lot of changes, we already know that we
>>> want to move away from port-based model in the OVS kernel module
>>> towards the flow model. As we push this upstream the userspace/kernel
>>> API should be the correct one, so that's why these two things are tied
>>> together.
>>
>>
>> Thanks, that explanation along with Kyle's response helps a lot.
>>
>> It seems to me that something I could help out with is the implementation
>> of the set_tunnel action which extents and replaces the tun_id action.
>> It seems that is a requirement for the scheme you describe above.
>>
>> http://openvswitch.org/pipermail/dev/2012-April/016239.html
>
> I agree that's probably the best place to start unless Kyle has some
> specific plans otherwise.
Simon and I chatted off-list, and this is indeed where we plan to start.
^ permalink raw reply
* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Jesse Gross @ 2012-04-26 16:13 UTC (permalink / raw)
To: Simon Horman
Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
shemminger-ZtmgI6mnKB3QT0dZR+AlfA, David Miller
In-Reply-To: <20120426071321.GA25781-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
On Thu, Apr 26, 2012 at 12:13 AM, Simon Horman <horms@verge.net.au> wrote:
> On Wed, Apr 25, 2012 at 10:17:25AM -0700, Jesse Gross wrote:
>> On Wed, Apr 25, 2012 at 1:39 AM, Simon Horman <horms@verge.net.au> wrote:
>> >
>> > Hi Kyle,
>> >
>> > the component that is of most interest to me is enabling OVS to use in-tree
>> > tunnelling code - as it seems that makes most sense for an implementation
>> > of STT. I have taken a brief look over your vxlan work and it isn't clear
>> > to me if it is moving towards being an in-tree implementation. Moreover,
>> > I'm a rather unclear on what changes need to be made to OVS in order for
>> > in-tree tunneling to be used.
>> >
>> > My recollection is that OVS did make use of in-tree tunnelling code
>> > but this was removed in favour of the current implementation for various
>> > reasons (performance being one IIRC). I gather that revisiting in-tree
>> > tunnelling won't revisit the previous set of problems. But I'm unclear how.
>> >
>> > Jesse, is it possible for you to describe that in a little detail
>> > or point me to some information?
>>
>> This was what I had originally written a while back, although it's
>> more about OVS internally and less about how to connect to the in-tree
>> code:
>> http://openvswitch.org/pipermail/dev/2012-February/014779.html
>>
>> In order to flexibly implement support for current and future tunnel
>> protocols OVS needs to be able to get/set information about the outer
>> tunnel header when processing the inner packet. At the very least
>> this is src/dst IP addresses and the key/ID/VNI/etc. In the upstream
>> tunnel implementations those are implicitly encoded in the device that
>> sends or receives the packet. However, this has a two problems:
>> number of devices and ability to handle unknown values. We addressed
>> part of this problem by allowing the tunnel ID to be set and matched
>> through the OVS flow table and an action. In order to do this with
>> the in-tree tunneling code, we obviously need a way of passing this
>> information around since it would currently get lost as we pass
>> through the Linux device layer.
>>
>> The plan to deal with that is to add a function to the in-tree
>> tunneling code that allows a skb to be encapsulated with specific
>> parameters and conversely a hook to receive decapsulated packets along
>> with header info. This would make all of the kernel tunneling code
>> common, while still giving OVS userspace the ability to implement
>> essentially any type of tunneling policy. In many ways, this is very
>> similar to how vlans look in OVS today.
>>
>> While it would be possible to implement the hook to use the in-tree
>> tunnel code today without a lot of changes, we already know that we
>> want to move away from port-based model in the OVS kernel module
>> towards the flow model. As we push this upstream the userspace/kernel
>> API should be the correct one, so that's why these two things are tied
>> together.
>
>
> Thanks, that explanation along with Kyle's response helps a lot.
>
> It seems to me that something I could help out with is the implementation
> of the set_tunnel action which extents and replaces the tun_id action.
> It seems that is a requirement for the scheme you describe above.
>
> http://openvswitch.org/pipermail/dev/2012-April/016239.html
I agree that's probably the best place to start unless Kyle has some
specific plans otherwise.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Re: Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Eric Dumazet @ 2012-04-26 15:53 UTC (permalink / raw)
To: Shawn Bohrer; +Cc: netdev
In-Reply-To: <20120426151527.GA2479@BohrerMBP.rgmadvisors.com>
On Thu, 2012-04-26 at 10:15 -0500, Shawn Bohrer wrote:
> I've been doing some UDP multicast benchmarking and noticed that as we
> increase the number of sockets/multicast addresses the performance
> degrades. The test I'm running has multiple machines sending packets
> on multiple multicast addresses. A single receiving machine opens one
> socket per multicast address to receive all the packets. The
> receiving process is bound to a core that is not processing
> interrupts.
>
> Running this test with 300 multicast addresses and sockets and
> profiling the receiving machine with 'perf -a -g' I can see the
> following:
>
>
> # Events: 45K cycles
> #
> # Overhead
> # ........ .....................................
> #
> 52.56% [k] _raw_spin_lock
> |
> |--99.09%-- __udp4_lib_mcast_deliver
> 20.10% [k] __udp4_lib_mcast_deliver
> |
> --- __udp4_lib_rcv
>
> So if I understand this correctly 52.56% of the time is spent
> contending for the spin_lock in __udp4_lib_mcast_deliver. If I
> understand the code correctly it appears that for every packet
> received we walk the list of all UDP sockets while holding the
> spin_lock. Therefore I believe the thing that hurts so much in this
> case is that we have a lot of UDP sockets.
>
> Are there any ideas on how we can improve the performance in this
> case? Honestly I have two ideas though my understanding of the
> network stack is limited and it is unclear to me how to implement
> either of them.
>
> The first idea is to use RCU instead of acquiring the spin_lock. This
> is what the Unicast path does however looking back to 271b72c7 "udp:
> RCU handling for Unicast packets." Eric points out that the multicast
> path is difficult. It appears from that commit description that the
> problem is that since we have to find all sockets interested in
> receiving the packet instead of just one that restarting the scan of
> the hlist could lead us to deliver the packet twice to the same
> socket. That commit is rather old though I believe things may have
> changed. Looking at commit 1240d137 "ipv4: udp: Optimise multicast
> reception" I can see that Eric also has already done some work to
> reduce how long the spin_lock is held in __udp4_lib_mcast_deliver().
> That commit also says "It's also a base for a future RCU conversion of
> multicast recption". Is the idea that you could remove duplicate
> sockets within flush_stack()? Actually I don't think that would work
> since flush_stack() can be called multiple times if the stack gets
> full.
>
> The second idea would be to hash the sockets to reduce the number of
> sockets to walk for each packet. Once again it looks like the Unicast
> path already does this in commits 512615b6b "udp: secondary hash on
> (local port, local address)" and 5051ebd27 "ipv4: udp: optimize
> unicast RX path". Perhaps these hash lists could be used, however I
> don't think they can since they currently use RCU and thus it might
> depend on converting to RCU first.
Let me understand
You have 300 sockets bound to the same port, so a single message must be
copied 300 times and delivered to those sockets ?
^ permalink raw reply
* Re: [v2 PATCH 1/1] ipvs: kernel oops - do_ip_vs_get_ctl
From: Pablo Neira Ayuso @ 2012-04-26 15:26 UTC (permalink / raw)
To: Simon Horman
Cc: Jesper Dangaard Brouer, Hans Schillstrom, ja, wensong, rohara,
lvs-devel, netdev, netfilter-devel, hans
In-Reply-To: <20120426090520.GA5801@verge.net.au>
On Thu, Apr 26, 2012 at 06:05:20PM +0900, Simon Horman wrote:
> On Thu, Apr 26, 2012 at 10:10:41AM +0200, Jesper Dangaard Brouer wrote:
> > On Thu, 2012-04-26 at 07:47 +0200, Hans Schillstrom wrote:
> > > Change order of init so netns init is ready
> > > when register ioctl and netlink.
> > >
> > > Ver2
> > > Whitespace fixes and __init added.
> > >
> > > Reported-by: "Ryan O'Hara" <rohara@redhat.com>
> > > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
> >
> > Looks good, interdiff says you fixed up things, and I have tested it
> > against the net tree, good work :-)
> >
> > Is this a stable tree candidate?
>
> Yes, I think so.
>
> I now have the following 7 changes in my ipvs tree on kernel.org.
>
> The last two are already in Pablo's net tree and I intend
> to send a pull request for the remaining 5 soon.
OK, then I'll wait for that pull request to pass these 7 patch batch
to davem if you make it anytime soon.
> It is my opinion that all 7 patches are stable tree material.
Little reminder: We can ask for this once they have hit Linus' tree.
^ permalink raw reply
* Heavy spin_lock contention in __udp4_lib_mcast_deliver increase
From: Shawn Bohrer @ 2012-04-26 15:15 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet
I've been doing some UDP multicast benchmarking and noticed that as we
increase the number of sockets/multicast addresses the performance
degrades. The test I'm running has multiple machines sending packets
on multiple multicast addresses. A single receiving machine opens one
socket per multicast address to receive all the packets. The
receiving process is bound to a core that is not processing
interrupts.
Running this test with 300 multicast addresses and sockets and
profiling the receiving machine with 'perf -a -g' I can see the
following:
# Events: 45K cycles
#
# Overhead
# ........ .....................................
#
52.56% [k] _raw_spin_lock
|
|--99.09%-- __udp4_lib_mcast_deliver
20.10% [k] __udp4_lib_mcast_deliver
|
--- __udp4_lib_rcv
So if I understand this correctly 52.56% of the time is spent
contending for the spin_lock in __udp4_lib_mcast_deliver. If I
understand the code correctly it appears that for every packet
received we walk the list of all UDP sockets while holding the
spin_lock. Therefore I believe the thing that hurts so much in this
case is that we have a lot of UDP sockets.
Are there any ideas on how we can improve the performance in this
case? Honestly I have two ideas though my understanding of the
network stack is limited and it is unclear to me how to implement
either of them.
The first idea is to use RCU instead of acquiring the spin_lock. This
is what the Unicast path does however looking back to 271b72c7 "udp:
RCU handling for Unicast packets." Eric points out that the multicast
path is difficult. It appears from that commit description that the
problem is that since we have to find all sockets interested in
receiving the packet instead of just one that restarting the scan of
the hlist could lead us to deliver the packet twice to the same
socket. That commit is rather old though I believe things may have
changed. Looking at commit 1240d137 "ipv4: udp: Optimise multicast
reception" I can see that Eric also has already done some work to
reduce how long the spin_lock is held in __udp4_lib_mcast_deliver().
That commit also says "It's also a base for a future RCU conversion of
multicast recption". Is the idea that you could remove duplicate
sockets within flush_stack()? Actually I don't think that would work
since flush_stack() can be called multiple times if the stack gets
full.
The second idea would be to hash the sockets to reduce the number of
sockets to walk for each packet. Once again it looks like the Unicast
path already does this in commits 512615b6b "udp: secondary hash on
(local port, local address)" and 5051ebd27 "ipv4: udp: optimize
unicast RX path". Perhaps these hash lists could be used, however I
don't think they can since they currently use RCU and thus it might
depend on converting to RCU first.
Thanks,
Shawn
--
---------------------------------------------------------------
This email, along with any attachments, is confidential. If you
believe you received this message in error, please contact the
sender immediately and delete all copies of the message.
Thank you.
^ permalink raw reply
* Re: [PATCH 1/2] iproute2: allow IPv6 addresses for l2tp local and remote parameters
From: Stephen Hemminger @ 2012-04-26 15:11 UTC (permalink / raw)
To: James Chapman; +Cc: netdev, Chris Elston
In-Reply-To: <4F990815.9000007@katalix.com>
On Thu, 26 Apr 2012 09:32:21 +0100
James Chapman <jchapman@katalix.com> wrote:
> On 25/04/12 21:13, Stephen Hemminger wrote:
> > On Fri, 20 Apr 2012 12:29:42 +0100
> > James Chapman <jchapman@katalix.com> wrote:
> >
> >> From: Chris Elston <celston@katalix.com>
> >>
> >> Adds support for parsing IPv6 addresses to the parameters local and
> >> remote in the l2tp commands. Requires netlink attributes L2TP_ATTR_IP6_SADDR
> >> and L2TP_ATTR_IP6_DADDR, added in a required kernel patch already submitted
> >> to netdev.
> >>
> >> Also enables printing of IPv6 addresses returned by the L2TP_CMD_TUNNEL_GET
> >> request.
> >>
> >> Signed-off-by: Chris Elston <celston@katalix.com>
> >> Signed-off-by: James Chapman <jchapman@katalix.com>
> >
> > Accepted.
>
> The kernel patches that this code depends on aren't in the tree yet
> (kernel header file update). It uses new netlink attributes so the
> iproute2 git compile fails.
>
> I suggest revert the patch for now. I'll resubmit when the dependencies
> are in the netdev tree.
>
>
Okay thanks.
^ permalink raw reply
* Re: [PATCH] mISDN: Ignore return value of driver_for_each_device in function hfcpci_softirq.
From: Krzysztof Wilczynski @ 2012-04-26 15:08 UTC (permalink / raw)
To: David Miller; +Cc: isdn, gregkh, horms, trivial, netdev, linux-kernel
In-Reply-To: <20120425.204231.1586199621022282253.davem@davemloft.net>
Hi,
[...]
> I'm not applying this, at least not with a more intelligent analysis
> of why any error or other non-zero value signalled by
> driver_for_each_device should be completely ignored, and no error
> handling should be performed.
This is a fair point. It should not be ignored. I was looking through
the code and found few cases where the return value was not checked
for potential errors (I assume that it was collected to avoid warning being
produced, but that was about it) without printing an error message.
The following could most certainly use better error reporting concerning
use of driver_for_each_device:
drivers/isdn/hardware/mISDN/hfcpci.c
drivers/media/video/cx18/cx18-alsa-main.c
drivers/media/video/ivtv/ivtvfb.c
I will look into it making suitable changes. Thanks for pointing it
out to me! :)
KW
^ permalink raw reply
* [PATCH net-next 1/5] be2net: Fix to not set link speed for disabled functions of a UMC card
From: Somnath Kotur @ 2012-04-26 14:30 UTC (permalink / raw)
To: netdev; +Cc: Somnath Kotur
From: Somnath Kotur <somnath.kotur@emulex.com>
This renders the interface view somewhat inconsistent from the Host OS POV
considering the rest of the interfaces are showing their respective speeds
based on the bandwidth assigned to them.
Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c
index 0c6f06e..a00983e 100644
--- a/drivers/net/ethernet/emulex/benet/be_ethtool.c
+++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c
@@ -558,7 +558,7 @@ static int be_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
be_link_status_update(adapter, link_status);
if (link_speed)
et_speed = link_speed;
- else
+ else if (link_status)
et_speed = convert_to_et_speed(port_speed);
} else {
et_speed = adapter->phy.forced_port_speed;
--
1.5.6.1
^ permalink raw reply related
* [PATCH net-next 5/5] be2net: Fix EEH error reset before a flash dump completes
From: Somnath Kotur @ 2012-04-26 14:29 UTC (permalink / raw)
To: netdev; +Cc: Somnath Kotur, Sathya Perla
From: Somnath Kotur <somnath.kotur@emulex.com>
An EEH error can cause the FW to trigger a flash debug dump.
Resetting the card while flash dump is in progress can cause it not to recover.
Wait for it to finish before letting EEH flow to reset the card.
Signed-off-by: Sathya Perla <Sathya.Perla@emulex.com>
Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 602ba6c..435e2ed 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -3830,6 +3830,11 @@ static pci_ers_result_t be_eeh_err_detected(struct pci_dev *pdev,
pci_disable_device(pdev);
+ /* The error could cause the FW to trigger a flash debug dump.
+ * Resetting the card while flash dump is in progress
+ * can cause it not to recover; wait for it to finish
+ */
+ ssleep(30);
return PCI_ERS_RESULT_NEED_RESET;
}
--
1.5.6.1
^ permalink raw reply related
* [PATCH net-next 4/5] be2net: Record receive queue index in skb to aid RPS.
From: Somnath Kotur @ 2012-04-26 14:28 UTC (permalink / raw)
To: netdev; +Cc: Somnath Kotur
From: Somnath Kotur <somnath.kotur@emulex.com>
Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_main.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index c9f757c..602ba6c 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -1259,6 +1259,7 @@ static void be_rx_compl_process(struct be_rx_obj *rxo,
skb_checksum_none_assert(skb);
skb->protocol = eth_type_trans(skb, netdev);
+ skb_record_rx_queue(skb, rxo - &adapter->rx_obj[0]);
if (netdev->features & NETIF_F_RXHASH)
skb->rxhash = rxcp->rss_hash;
@@ -1315,6 +1316,7 @@ void be_rx_compl_process_gro(struct be_rx_obj *rxo, struct napi_struct *napi,
skb->len = rxcp->pkt_size;
skb->data_len = rxcp->pkt_size;
skb->ip_summed = CHECKSUM_UNNECESSARY;
+ skb_record_rx_queue(skb, rxo - &adapter->rx_obj[0]);
if (adapter->netdev->features & NETIF_F_RXHASH)
skb->rxhash = rxcp->rss_hash;
--
1.5.6.1
^ permalink raw reply related
* [PATCH net-next 3/5] be2net: Fix to apply duplex value as unknown when link is down.
From: Somnath Kotur @ 2012-04-26 14:28 UTC (permalink / raw)
To: netdev; +Cc: Somnath Kotur
From: Somnath Kotur <somnath.kotur@emulex.com>
Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c
index 5296df5..bb2add7 100644
--- a/drivers/net/ethernet/emulex/benet/be_ethtool.c
+++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c
@@ -618,7 +618,7 @@ static int be_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
ecmd->supported = adapter->phy.supported;
}
- ecmd->duplex = DUPLEX_FULL;
+ ecmd->duplex = netif_carrier_ok(netdev) ? DUPLEX_FULL : -1;
ecmd->phy_address = adapter->port_num;
return 0;
--
1.5.6.1
^ permalink raw reply related
* [PATCH net-next 2/5] be2net: Fix to allow setting of debug levels in the firmware.
From: Somnath Kotur @ 2012-04-26 14:27 UTC (permalink / raw)
To: netdev; +Cc: Somnath Kotur
From: Somnath Kotur <somnath.kotur@emulex.com>
Signed-off-by: Somnath Kotur <somnath.kotur@emulex.com>
---
drivers/net/ethernet/emulex/benet/be_cmds.c | 59 ++++++++++++++++++++
drivers/net/ethernet/emulex/benet/be_cmds.h | 69 ++++++++++++++++++++++++
drivers/net/ethernet/emulex/benet/be_ethtool.c | 67 +++++++++++++++++++++++
3 files changed, 195 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index 43167e8..9971e4b 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -2590,3 +2590,62 @@ err:
pci_free_consistent(adapter->pdev, cmd.size, cmd.va, cmd.dma);
return status;
}
+
+int be_cmd_get_ext_fat_capabilites(struct be_adapter *adapter,
+ struct be_dma_mem *cmd)
+{
+ struct be_mcc_wrb *wrb;
+ struct be_cmd_req_get_ext_fat_caps *req;
+ int status;
+
+
+ spin_lock_bh(&adapter->mcc_lock);
+
+ wrb = wrb_from_mccq(adapter);
+ if (!wrb) {
+ status = -EBUSY;
+ goto err;
+ }
+
+ req = cmd->va;
+ be_wrb_cmd_hdr_prepare(&req->hdr, CMD_SUBSYSTEM_COMMON,
+ OPCODE_COMMON_GET_EXT_FAT_CAPABILITES,
+ cmd->size, wrb, cmd);
+ /*
+ * Retrieve currently set parameters
+ */
+ req->parameter_type = cpu_to_le32(1);
+
+ status = be_mcc_notify_wait(adapter);
+err:
+ spin_unlock_bh(&adapter->mcc_lock);
+ return status;
+}
+
+int be_cmd_set_ext_fat_capabilites(struct be_adapter *adapter,
+ struct be_dma_mem *cmd,
+ struct be_fat_conf_params *configs)
+{
+ struct be_mcc_wrb *wrb;
+ struct be_cmd_req_set_ext_fat_caps *req;
+ int status;
+
+ spin_lock_bh(&adapter->mcc_lock);
+
+ wrb = wrb_from_mccq(adapter);
+ if (!wrb) {
+ status = -EBUSY;
+ goto err;
+ }
+
+ req = cmd->va;
+ memcpy(&req->set_params, configs, sizeof(struct be_fat_conf_params));
+ be_wrb_cmd_hdr_prepare(&req->hdr, CMD_SUBSYSTEM_COMMON,
+ OPCODE_COMMON_SET_EXT_FAT_CAPABILITES,
+ cmd->size, wrb, cmd);
+
+ status = be_mcc_notify_wait(adapter);
+err:
+ spin_unlock_bh(&adapter->mcc_lock);
+ return status;
+}
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.h b/drivers/net/ethernet/emulex/benet/be_cmds.h
index 944f031..02df9f8 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.h
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.h
@@ -189,6 +189,8 @@ struct be_mcc_mailbox {
#define OPCODE_COMMON_GET_PHY_DETAILS 102
#define OPCODE_COMMON_SET_DRIVER_FUNCTION_CAP 103
#define OPCODE_COMMON_GET_CNTL_ADDITIONAL_ATTRIBUTES 121
+#define OPCODE_COMMON_GET_EXT_FAT_CAPABILITES 125
+#define OPCODE_COMMON_SET_EXT_FAT_CAPABILITES 126
#define OPCODE_COMMON_GET_MAC_LIST 147
#define OPCODE_COMMON_SET_MAC_LIST 148
#define OPCODE_COMMON_GET_HSW_CONFIG 152
@@ -1602,6 +1604,68 @@ static inline void *be_erx_stats_from_cmd(struct be_adapter *adapter)
}
}
+/************** get fat capabilites *******************/
+#define MAX_MODULES 27
+#define MAX_MODES 4
+#define MODE_UART 0
+
+/* Trace level Valid values */
+enum {
+ TRACE_LEVEL_ALL = 0x0,
+ TRACE_LEVEL_ENTER_FUNCTION = 0x1,
+ TRACE_LEVEL_VERBOSE = 0x4,
+ TRACE_LEVEL_INFO = 0x5,
+ TRACE_LEVEL_TEST = 0x8,
+ TRACE_LEVEL_SHOW = 0x10,
+ TRACE_LEVEL_WARNING = 0x20,
+ TRACE_LEVEL_ERROR = 0x30,
+ TRACE_LEVEL_FATAL = 0x40,
+ TRACE_LEVEL_ALWAYS = 0x80,
+ TRACE_LEVEL_DISABLED = 0xff
+};
+
+struct ext_fat_mode {
+ u8 mode;
+ u8 rsvd0;
+ u16 port_mask;
+ u32 dbg_lvl;
+ u64 fun_mask;
+} __packed;
+
+struct ext_fat_modules {
+ u8 modules_str[32];
+ u32 modules_id;
+ u32 num_modes;
+ struct ext_fat_mode trace_lvl[MAX_MODES];
+} __packed;
+
+struct be_fat_conf_params {
+ u32 max_log_entries;
+ u32 log_entry_size;
+ u8 log_type;
+ u8 max_log_funs;
+ u8 max_log_ports;
+ u8 rsvd0;
+ u32 supp_modes;
+ u32 num_modules;
+ struct ext_fat_modules module[MAX_MODULES];
+} __packed;
+
+struct be_cmd_req_get_ext_fat_caps {
+ struct be_cmd_req_hdr hdr;
+ u32 parameter_type;
+};
+
+struct be_cmd_resp_get_ext_fat_caps {
+ struct be_cmd_resp_hdr hdr;
+ struct be_fat_conf_params get_params;
+};
+
+struct be_cmd_req_set_ext_fat_caps {
+ struct be_cmd_req_hdr hdr;
+ struct be_fat_conf_params set_params;
+};
+
extern int be_pci_fnum_get(struct be_adapter *adapter);
extern int be_cmd_POST(struct be_adapter *adapter);
extern int be_cmd_mac_addr_query(struct be_adapter *adapter, u8 *mac_addr,
@@ -1707,4 +1771,9 @@ extern int be_cmd_set_hsw_config(struct be_adapter *adapter, u16 pvid,
extern int be_cmd_get_hsw_config(struct be_adapter *adapter, u16 *pvid,
u32 domain, u16 intf_id);
extern int be_cmd_get_acpi_wol_cap(struct be_adapter *adapter);
+extern int be_cmd_get_ext_fat_capabilites(struct be_adapter *adapter,
+ struct be_dma_mem *cmd);
+extern int be_cmd_set_ext_fat_capabilites(struct be_adapter *adapter,
+ struct be_dma_mem *cmd,
+ struct be_fat_conf_params *cfgs);
diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c
index a00983e..5296df5 100644
--- a/drivers/net/ethernet/emulex/benet/be_ethtool.c
+++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c
@@ -878,6 +878,72 @@ be_read_eeprom(struct net_device *netdev, struct ethtool_eeprom *eeprom,
return status;
}
+static void be_set_msglevel(struct net_device *netdev, u32 data)
+{
+ struct be_adapter *adapter = netdev_priv(netdev);
+ struct be_dma_mem extfat_cmd;
+ struct be_fat_conf_params *cfgs;
+ int status;
+ int i, j;
+
+ if (lancer_chip(adapter)) {
+ dev_err(&adapter->pdev->dev, "Operation not supported\n");
+ return;
+ }
+
+ switch (data) {
+ case TRACE_LEVEL_ALL:
+ case TRACE_LEVEL_ENTER_FUNCTION:
+ case TRACE_LEVEL_VERBOSE:
+ case TRACE_LEVEL_INFO:
+ case TRACE_LEVEL_TEST:
+ case TRACE_LEVEL_SHOW:
+ case TRACE_LEVEL_WARNING:
+ case TRACE_LEVEL_ERROR:
+ case TRACE_LEVEL_FATAL:
+ case TRACE_LEVEL_ALWAYS:
+ case TRACE_LEVEL_DISABLED:
+ break;
+ default:
+ dev_err(&adapter->pdev->dev, "Invalid message level value\n");
+ return;
+ }
+
+ memset(&extfat_cmd, 0, sizeof(struct be_dma_mem));
+ extfat_cmd.size = sizeof(struct be_cmd_resp_get_ext_fat_caps);
+ extfat_cmd.va = pci_alloc_consistent(adapter->pdev, extfat_cmd.size,
+ &extfat_cmd.dma);
+ if (!extfat_cmd.va) {
+ dev_err(&adapter->pdev->dev, "%s: Memory allocation failure\n",
+ __func__);
+ goto err;
+ }
+ status = be_cmd_get_ext_fat_capabilites(adapter, &extfat_cmd);
+ if (!status) {
+ cfgs = (struct be_fat_conf_params *)(extfat_cmd.va +
+ sizeof(struct be_cmd_resp_hdr));
+ for (i = 0; i < cfgs->num_modules; i++) {
+ for (j = 0; j < cfgs->module[i].num_modes; j++) {
+ if (cfgs->module[i].trace_lvl[j].mode ==
+ MODE_UART)
+ cfgs->module[i].trace_lvl[j].dbg_lvl =
+ cpu_to_le32(data);
+ }
+ }
+ status = be_cmd_set_ext_fat_capabilites(adapter, &extfat_cmd,
+ cfgs);
+ if (status)
+ dev_err(&adapter->pdev->dev,
+ "Message level set failed\n");
+ } else {
+ dev_err(&adapter->pdev->dev, "Message level get failed\n");
+ }
+
+ pci_free_consistent(adapter->pdev, extfat_cmd.size, extfat_cmd.va,
+ extfat_cmd.dma);
+err:
+ return;
+}
const struct ethtool_ops be_ethtool_ops = {
.get_settings = be_get_settings,
.get_drvinfo = be_get_drvinfo,
@@ -893,6 +959,7 @@ const struct ethtool_ops be_ethtool_ops = {
.set_pauseparam = be_set_pauseparam,
.get_strings = be_get_stat_strings,
.set_phys_id = be_set_phys_id,
+ .set_msglevel = be_set_msglevel,
.get_sset_count = be_get_sset_count,
.get_ethtool_stats = be_get_ethtool_stats,
.get_regs_len = be_get_reg_len,
--
1.5.6.1
^ 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