* Re: [PATCH] b44: properly use pr_fmt()
From: Luis R. Rodriguez @ 2012-04-26 13:02 UTC (permalink / raw)
To: David Miller
Cc: zambrano-dY08KVG/lbpWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless, linux-bluetooth, Arend Van Spriel, Joe Perches
In-Reply-To: <CAB=NE6WBw3aJrch0JvxZ7jGb_rFW5Z4XWvYc1KqsBqBu+G8mjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Apr 26, 2012 at 5:23 AM, Luis R. Rodriguez
<mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org> wrote:
> On Mon, Apr 23, 2012 at 9:47 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> From: "Luis R. Rodriguez" <mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
>> Date: Mon, 23 Apr 2012 19:46:43 -0700
>>
>>> From: "Luis R. Rodriguez" <mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
>>>
>>> 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-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
>>
>> 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.
Luis
^ permalink raw reply
* Re: [RFC] allow skb->head to point/alias to first skb frag
From: Eric Dumazet @ 2012-04-26 13:50 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: David Miller, ilpo.jarvinen, rick.jones2, netdev, therbert,
ncardwell, ycheng
In-Reply-To: <1335443554.2775.33.camel@edumazet-glaptop>
On Thu, 2012-04-26 at 14:32 +0200, Eric Dumazet wrote:
> On Thu, 2012-04-26 at 03:09 -0700, Maciej Żenczykowski wrote:
> > it would be very useful if there was an api for freeing skb->head memory...
> >
> > I've fooled around with a single memory buffer for both the head and
> > the data and seen huge performance wins under heavy packet load (half
> > the amount of malloc/free), but could never quite get it to be 100%
> > stable.
>
> My patch is almost ready, and seems very good.
>
> I added a param to skb_build() to tell the memory comes from a (sub)page
>
> tg3 uses the new thing.
>
> GRO does the correct merging.
>
> (by the way, I noticed GRO lies about skb truesize... since it
> accumulates frag lengths instead of allocated space... with a 1448/2048
> mismatch for MTU=1500)
>
> And this apparently solves my slub performance issue I had on my dual
> quad core machine, since I no longer hit slub slow path.
>
>
Here is the raw patch. (tg3 part is dirty and needs some helpers)
I am working on the TCP coalesce part, the splice() helpers (to avoid a
copy), and split in five different patches for further
discussion/inclusion.
drivers/net/ethernet/broadcom/bnx2.c | 2
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 4
drivers/net/ethernet/broadcom/tg3.c | 61 +++++++++++---
drivers/net/ethernet/broadcom/tg3.h | 2
include/linux/skbuff.h | 7 +
net/core/dev.c | 5 -
net/core/skbuff.c | 46 ++++++++--
net/ipv4/tcp_input.c | 1
8 files changed, 103 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index ab55979..ac7b744 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -3006,7 +3006,7 @@ error:
dma_unmap_single(&bp->pdev->dev, dma_addr, bp->rx_buf_use_size,
PCI_DMA_FROMDEVICE);
- skb = build_skb(data);
+ skb = build_skb(data, 0);
if (!skb) {
kfree(data);
goto error;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 60d5b54..9c5bc5d 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -513,7 +513,7 @@ static inline void bnx2x_tpa_stop(struct bnx2x *bp, struct bnx2x_fastpath *fp,
dma_unmap_single(&bp->pdev->dev, dma_unmap_addr(rx_buf, mapping),
fp->rx_buf_size, DMA_FROM_DEVICE);
if (likely(new_data))
- skb = build_skb(data);
+ skb = build_skb(data, 0);
if (likely(skb)) {
#ifdef BNX2X_STOP_ON_ERROR
@@ -721,7 +721,7 @@ int bnx2x_rx_int(struct bnx2x_fastpath *fp, int budget)
dma_unmap_addr(rx_buf, mapping),
fp->rx_buf_size,
DMA_FROM_DEVICE);
- skb = build_skb(data);
+ skb = build_skb(data, 0);
if (unlikely(!skb)) {
kfree(data);
fp->eth_q_stats.rx_skb_alloc_failed++;
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 0c3e7c7..d216076 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -5619,12 +5619,18 @@ static void tg3_tx(struct tg3_napi *tnapi)
static void tg3_rx_data_free(struct tg3 *tp, struct ring_info *ri, u32 map_sz)
{
+ unsigned int skb_size = SKB_DATA_ALIGN(map_sz + TG3_RX_OFFSET(tp)) +
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+
if (!ri->data)
return;
pci_unmap_single(tp->pdev, dma_unmap_addr(ri, mapping),
map_sz, PCI_DMA_FROMDEVICE);
- kfree(ri->data);
+ if (skb_size <= 2048)
+ put_page(virt_to_head_page(ri->data));
+ else
+ kfree(ri->data);
ri->data = NULL;
}
@@ -5640,7 +5646,8 @@ static void tg3_rx_data_free(struct tg3 *tp, struct ring_info *ri, u32 map_sz)
* (to fetch the error flags, vlan tag, checksum, and opaque cookie).
*/
static int tg3_alloc_rx_data(struct tg3 *tp, struct tg3_rx_prodring_set *tpr,
- u32 opaque_key, u32 dest_idx_unmasked)
+ u32 opaque_key, u32 dest_idx_unmasked,
+ unsigned int *frag_size)
{
struct tg3_rx_buffer_desc *desc;
struct ring_info *map;
@@ -5675,16 +5682,36 @@ static int tg3_alloc_rx_data(struct tg3 *tp, struct tg3_rx_prodring_set *tpr,
*/
skb_size = SKB_DATA_ALIGN(data_size + TG3_RX_OFFSET(tp)) +
SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
- data = kmalloc(skb_size, GFP_ATOMIC);
- if (!data)
- return -ENOMEM;
-
+ if (skb_size <= 2048) {
+ if (tpr->rx_page_size < 2048) {
+ struct page *page = alloc_page(GFP_ATOMIC);
+ if (!page)
+ return -ENOMEM;
+ atomic_add((PAGE_SIZE / 2048) - 1, &page->_count);
+ tpr->rx_page_addr = page_address(page);
+ tpr->rx_page_size = PAGE_SIZE;
+ }
+ data = tpr->rx_page_addr;
+ tpr->rx_page_addr += 2048;
+ tpr->rx_page_size -= 2048;
+ *frag_size = 2048;
+ } else {
+ data = kmalloc(skb_size, GFP_ATOMIC);
+ if (!data)
+ return -ENOMEM;
+ *frag_size = 0;
+ }
mapping = pci_map_single(tp->pdev,
data + TG3_RX_OFFSET(tp),
data_size,
PCI_DMA_FROMDEVICE);
if (pci_dma_mapping_error(tp->pdev, mapping)) {
- kfree(data);
+ if (skb_size <= 2048) {
+ tpr->rx_page_addr -= 2048;
+ tpr->rx_page_size += 2048;
+ } else {
+ kfree(data);
+ }
return -EIO;
}
@@ -5835,18 +5862,22 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
if (len > TG3_RX_COPY_THRESH(tp)) {
int skb_size;
+ unsigned int frag_size;
skb_size = tg3_alloc_rx_data(tp, tpr, opaque_key,
- *post_ptr);
+ *post_ptr, &frag_size);
if (skb_size < 0)
goto drop_it;
pci_unmap_single(tp->pdev, dma_addr, skb_size,
PCI_DMA_FROMDEVICE);
- skb = build_skb(data);
+ skb = build_skb(data, frag_size);
if (!skb) {
- kfree(data);
+ if (frag_size)
+ put_page(virt_to_head_page(data));
+ else
+ kfree(data);
goto drop_it_no_recycle;
}
skb_reserve(skb, TG3_RX_OFFSET(tp));
@@ -7279,7 +7310,10 @@ static int tg3_rx_prodring_alloc(struct tg3 *tp,
/* Now allocate fresh SKBs for each rx ring. */
for (i = 0; i < tp->rx_pending; i++) {
- if (tg3_alloc_rx_data(tp, tpr, RXD_OPAQUE_RING_STD, i) < 0) {
+ unsigned int frag_size;
+
+ if (tg3_alloc_rx_data(tp, tpr, RXD_OPAQUE_RING_STD, i,
+ &frag_size) < 0) {
netdev_warn(tp->dev,
"Using a smaller RX standard ring. Only "
"%d out of %d buffers were allocated "
@@ -7311,7 +7345,10 @@ static int tg3_rx_prodring_alloc(struct tg3 *tp,
}
for (i = 0; i < tp->rx_jumbo_pending; i++) {
- if (tg3_alloc_rx_data(tp, tpr, RXD_OPAQUE_RING_JUMBO, i) < 0) {
+ unsigned int frag_size;
+
+ if (tg3_alloc_rx_data(tp, tpr, RXD_OPAQUE_RING_JUMBO, i,
+ &frag_size) < 0) {
netdev_warn(tp->dev,
"Using a smaller RX jumbo ring. Only %d "
"out of %d buffers were allocated "
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index 93865f8..7c85545 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -2815,6 +2815,8 @@ struct tg3_rx_prodring_set {
struct ring_info *rx_jmb_buffers;
dma_addr_t rx_std_mapping;
dma_addr_t rx_jmb_mapping;
+ void *rx_page_addr;
+ unsigned int rx_page_size;
};
#define TG3_IRQ_MAX_VECS_RSS 5
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 4a656b5..1161111 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -470,7 +470,8 @@ struct sk_buff {
__u8 wifi_acked_valid:1;
__u8 wifi_acked:1;
__u8 no_fcs:1;
- /* 9/11 bit hole (depending on ndisc_nodetype presence) */
+ __u8 head_frag:1;
+ /* 8/10 bit hole (depending on ndisc_nodetype presence) */
kmemcheck_bitfield_end(flags2);
#ifdef CONFIG_NET_DMA
@@ -558,11 +559,13 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
}
extern void kfree_skb(struct sk_buff *skb);
+extern struct kmem_cache *skbuff_head_cache;
+
extern void consume_skb(struct sk_buff *skb);
extern void __kfree_skb(struct sk_buff *skb);
extern struct sk_buff *__alloc_skb(unsigned int size,
gfp_t priority, int fclone, int node);
-extern struct sk_buff *build_skb(void *data);
+extern struct sk_buff *build_skb(void *data, unsigned int frag_size);
static inline struct sk_buff *alloc_skb(unsigned int size,
gfp_t priority)
{
diff --git a/net/core/dev.c b/net/core/dev.c
index 501f3cc..9537321 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3546,7 +3546,10 @@ gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
break;
case GRO_MERGED_FREE:
- consume_skb(skb);
+ if (skb->head_frag)
+ kmem_cache_free(skbuff_head_cache, skb);
+ else
+ consume_skb(skb);
break;
case GRO_HELD:
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2342a72..d8fb4b2 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -69,7 +69,7 @@
#include <trace/events/skb.h>
#include <linux/highmem.h>
-static struct kmem_cache *skbuff_head_cache __read_mostly;
+struct kmem_cache *skbuff_head_cache __read_mostly;
static struct kmem_cache *skbuff_fclone_cache __read_mostly;
static void sock_pipe_buf_release(struct pipe_inode_info *pipe,
@@ -245,6 +245,7 @@ EXPORT_SYMBOL(__alloc_skb);
/**
* build_skb - build a network buffer
* @data: data buffer provided by caller
+ * @frag_size: size of fragment, or 0 if head was kmalloced
*
* Allocate a new &sk_buff. Caller provides space holding head and
* skb_shared_info. @data must have been allocated by kmalloc()
@@ -258,20 +259,21 @@ EXPORT_SYMBOL(__alloc_skb);
* before giving packet to stack.
* RX rings only contains data buffers, not full skbs.
*/
-struct sk_buff *build_skb(void *data)
+struct sk_buff *build_skb(void *data, unsigned int frag_size)
{
struct skb_shared_info *shinfo;
struct sk_buff *skb;
- unsigned int size;
+ unsigned int size = frag_size ? : ksize(data);
skb = kmem_cache_alloc(skbuff_head_cache, GFP_ATOMIC);
if (!skb)
return NULL;
- size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+ size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
memset(skb, 0, offsetof(struct sk_buff, tail));
skb->truesize = SKB_TRUESIZE(size);
+ skb->head_frag = frag_size != 0;
atomic_set(&skb->users, 1);
skb->head = data;
skb->data = data;
@@ -376,6 +378,14 @@ static void skb_clone_fraglist(struct sk_buff *skb)
skb_get(list);
}
+static void skb_free_head(struct sk_buff *skb)
+{
+ if (skb->head_frag)
+ put_page(virt_to_head_page(skb->head));
+ else
+ kfree(skb->head);
+}
+
static void skb_release_data(struct sk_buff *skb)
{
if (!skb->cloned ||
@@ -402,7 +412,7 @@ static void skb_release_data(struct sk_buff *skb)
if (skb_has_frag_list(skb))
skb_drop_fraglist(skb);
- kfree(skb->head);
+ skb_free_head(skb);
}
}
@@ -644,6 +654,7 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
C(tail);
C(end);
C(head);
+ C(head_frag);
C(data);
C(truesize);
atomic_set(&n->users, 1);
@@ -940,7 +951,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
fastpath = atomic_read(&skb_shinfo(skb)->dataref) == delta;
}
- if (fastpath &&
+ if (fastpath && !skb->head_frag &&
size + sizeof(struct skb_shared_info) <= ksize(skb->head)) {
memmove(skb->head + size, skb_shinfo(skb),
offsetof(struct skb_shared_info,
@@ -967,7 +978,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
if (fastpath) {
- kfree(skb->head);
+ skb_free_head(skb);
} else {
/* copy this zero copy skb frags */
if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
@@ -985,6 +996,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
off = (data + nhead) - skb->head;
skb->head = data;
+ skb->head_frag = 0;
adjust_others:
skb->data += off;
#ifdef NET_SKBUFF_DATA_USES_OFFSET
@@ -2889,6 +2901,26 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
NAPI_GRO_CB(skb)->free = 1;
goto done;
+ } else if (skb->head_frag) {
+ int nr_frags = pinfo->nr_frags;
+ skb_frag_t *frag = pinfo->frags + nr_frags;
+ struct page *page = virt_to_head_page(skb->head);
+
+
+ if (nr_frags >= MAX_SKB_FRAGS)
+ return -E2BIG;
+ offset -= headlen;
+ offset += skb->head - (unsigned char *)page_address(page);
+
+ pinfo->nr_frags = nr_frags + 1;
+
+ frag->page.p = page;
+ frag->page_offset = offset;
+ skb_frag_size_set(frag, len);
+
+ /* note : napi_skb_finish() will free skb, not skb->head */
+ NAPI_GRO_CB(skb)->free = 1;
+ goto done;
} else if (skb_gro_len(p) != pinfo->gso_size)
return -E2BIG;
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c93b0cb..69d379a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4478,6 +4478,7 @@ merge:
TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
return true;
}
+ /* TODO : handle special case where from->frag_head is true */
if (skb_headlen(from) == 0 &&
!skb_has_frag_list(to) &&
!skb_has_frag_list(from) &&
^ permalink raw reply related
* Re: kernel BUG at include/net/netns/generic.h:41
From: Dave Jones @ 2012-04-26 14:06 UTC (permalink / raw)
To: Stanislav Kinsbursky
Cc: Myklebust, Trond, Linux Kernel, netdev@vger.kernel.org
In-Reply-To: <4F98EA72.6010300@parallels.com>
On Thu, Apr 26, 2012 at 10:25:54AM +0400, Stanislav Kinsbursky wrote:
> > > >
> > > > > 2) how often do you experience this problem? Not every boot, I suppose?
> > > > > 3) And is it easy for you to reproduce?
> > > >
> > > > It happens most the time, but not every time.
> > >
> > > Ok, I'll send the patch soon.
> > > Hope, it will help.
> >
> > I didn't see a follow-up patch on this, and I'm still seeing the BUG in rc4.
> >
> Dave, search for "SUNRPC: register PipeFS file system after pernet
> sybsystem" in your mail.
> You are in CC list.
ah, missed that somehow.
I see it's now in Linus' tree too,
Thanks,
Dave
^ permalink raw reply
* [PATCH net-next 0/5] be2net fixes
From: Somnath Kotur @ 2012-04-26 14:27 UTC (permalink / raw)
To: netdev; +Cc: Somnath Kotur
From: Somnath Kotur <somnath.kotur@emulex.com>
Please apply.
Thanks
Somnath
Somnath Kotur (5):
be2net: Fix to not set link speed for disabled functions of a UMC
card
be2net: Fix to allow setting of debug levels in the firmware.
be2net: Fix to apply duplex value as unknown when link is down.
be2net: Record receive queue index in skb to aid RPS.
be2net: Fix EEH error reset before a flash dump completes
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 | 71 +++++++++++++++++++++++-
drivers/net/ethernet/emulex/benet/be_main.c | 7 ++
4 files changed, 204 insertions(+), 2 deletions(-)
^ 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:27 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 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
* [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 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 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 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
* 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
* 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
* 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: [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
* 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: [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: [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: 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: 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: 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: 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: 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: [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: [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 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
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