* Re: 3.2.8/amd64 full interrupt hangs and deadlocks under big network copies (page allocation failure)
From: Eric Dumazet @ 2012-04-10 6:11 UTC (permalink / raw)
To: Marc MERLIN
Cc: David Miller, Larry.Finger, bhutchings, linux-wireless, netdev
In-Reply-To: <20120410051127.GA32048@merlins.org>
On Mon, 2012-04-09 at 22:11 -0700, Marc MERLIN wrote:
> On Tue, Apr 10, 2012 at 05:56:20AM +0200, Eric Dumazet wrote:
> > > What wireless device are we dealing with again?
> >
> > Problem seems related to tailroom needed by mac80211
> > (IEEE80211_ENCRYPT_TAILROOM = 18 bytes)
> >
> > So we must reallocate skb->head, thats impressive nobody cares.
> >
> > [ 3007.249687] ieee80211_skb_resize(skb=ffff8802329846e8) cloned=1 head_need=0 tail_need=18 skb->len=1494 ksize=4096 tailroom=0 headroom=2282
> > [ 3007.249693] ieee80211_skb_resize(skb=ffff8802329846e8) cloned=0 head_need=0 tail_need=0 skb->len=1526 ksize=8192 tailroom=64 headroom=2250
> >
> > Ouch... skb_tailroom() seems wrong ... it seems pskb_expand_head() is really suboptimal.
> >
> > It appears tcp_sendmsg() tries to fill skb completely, with no available tailroom :
> >
> > if (skb_tailroom(skb) > 0) {
> > /* We have some space in skb head. Superb! */
> > if (copy > skb_tailroom(skb))
> > copy = skb_tailroom(skb);
> > err = skb_add_data_nocache(sk, skb, from, copy);
> > if (err)
> > goto do_fault;
> > } else {
> >
> > Shouldnt we take into account dev->needed_tailroom ?
> >
> > I'll submit a pskb_expand_head() fix asap.
>
> Thanks for finding this.
>
> To answer an earlier question, I tried the non wireless case too.
>
> The problem is harder to reproduce over e1000e though, I just got two short
> hangs where my mouse cursor was hung for 5-10 seconds, but nothing in
> syslog/dmesg this time.
>
> I'm pretty sure this older log below did happen on e1000e with wireless disabled
> though (but it had a taint 'O'):
>
> If that helps, my earlier message had the traces below.
>
> I can report back when you have a patch you'd like me to try out.
Hi Marc
Please try following patch, as it solved the problem for me (no more
order-1 allocations in tx path)
Thanks !
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3337027..70a3f8d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -481,6 +481,7 @@ struct sk_buff {
union {
__u32 mark;
__u32 dropcount;
+ __u32 avail_size;
};
sk_buff_data_t transport_header;
@@ -1366,6 +1367,18 @@ static inline int skb_tailroom(const struct sk_buff *skb)
}
/**
+ * skb_availroom - bytes at buffer end
+ * @skb: buffer to check
+ *
+ * Return the number of bytes of free space at the tail of an sk_buff
+ * allocated by sk_stream_alloc()
+ */
+static inline int skb_availroom(const struct sk_buff *skb)
+{
+ return skb_is_nonlinear(skb) ? 0 : skb->avail_size - skb->len;
+}
+
+/**
* skb_reserve - adjust headroom
* @skb: buffer to alter
* @len: bytes to move
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index baf8d28..1887454 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -952,9 +952,11 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
goto adjust_others;
}
- data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
+ data = kmalloc(size + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
+ gfp_mask);
if (!data)
goto nodata;
+ size = ksize(data) - SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
/* Copy only real data... and, alas, header. This should be
* optimized for the cases when header is void.
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 5d54ed3..87f497f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -701,11 +701,12 @@ struct sk_buff *sk_stream_alloc_skb(struct sock *sk, int size, gfp_t gfp)
skb = alloc_skb_fclone(size + sk->sk_prot->max_header, gfp);
if (skb) {
if (sk_wmem_schedule(sk, skb->truesize)) {
+ skb_reserve(skb, sk->sk_prot->max_header);
/*
* Make sure that we have exactly size bytes
* available to the caller, no more, no less.
*/
- skb_reserve(skb, skb_tailroom(skb) - size);
+ skb->avail_size = size;
return skb;
}
__kfree_skb(skb);
@@ -995,10 +996,9 @@ new_segment:
copy = seglen;
/* Where to copy to? */
- if (skb_tailroom(skb) > 0) {
+ if (skb_availroom(skb) > 0) {
/* We have some space in skb head. Superb! */
- if (copy > skb_tailroom(skb))
- copy = skb_tailroom(skb);
+ copy = min_t(int, copy, skb_availroom(skb));
err = skb_add_data_nocache(sk, skb, from, copy);
if (err)
goto do_fault;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 364784a..376b2cf 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2060,7 +2060,7 @@ static void tcp_retrans_try_collapse(struct sock *sk, struct sk_buff *to,
/* Punt if not enough space exists in the first SKB for
* the data in the second
*/
- if (skb->len > skb_tailroom(to))
+ if (skb->len > skb_availroom(to))
break;
if (after(TCP_SKB_CB(skb)->end_seq, tcp_wnd_end(tp)))
^ permalink raw reply related
* Re: [PATCH] gianfar: add missing include
From: Richard Cochran @ 2012-04-10 6:36 UTC (permalink / raw)
To: Michael Neuling; +Cc: David S. Miller, linuxppc-dev, netdev, linux-next
In-Reply-To: <25411.1334031527@neuling.org>
On Tue, Apr 10, 2012 at 02:18:47PM +1000, Michael Neuling wrote:
> This is because of a missing include file from:
> 6663628 gianfar: Support the get_ts_info ethtool method.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
I did a poor job testing the non-x86 stuff in this series. Thanks for
the fix.
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply
* Re: [PATCH 09/14] usb/net: rndis: merge media type definitions
From: Jussi Kivilinna @ 2012-04-10 6:37 UTC (permalink / raw)
To: Linus Walleij
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
Greg Kroah-Hartman, David S. Miller, Felipe Balbi, Haiyang Zhang,
Wei Yongjun, Ben Hutchings
In-Reply-To: <1333874900-5072-1-git-send-email-linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Quoting Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> Let's have a unified table of RNDIS media. We used to have a similar
> table with NDIS_* prefix from the gadget driver, but since we're only
> using RNDIS in the kernel (IIRC NDIS, non-remote, is for the windows-
> internal network drivers so what do we care) let's prefix everything
> with RNDIS. Some of the definitions were conflicting, in one of the
> defines 0x0B is bearer "CO WAN" and in two others "BPC". Well I took
> the majority vote. Two definition of medium 0x09 calls it "wireless
> WAN" but one vote for "wireless LAN" but in this case I am sticking
> with the minority, "Wide Area Network" does not make much sense in
> this case as far as I can tell.
>
> NOTE: latin singular and plural is so screwed up in these defines
> that it makes my eyes bleed. But I will not attempt to submit a
> patch converting all use of _MEDIA_ to _MEDIUM_ while I can probably
> tell from the semantics of the code that RNDIS_MEDIA_STATE_CONNECTED
> is most probably (erroneously) referring to a singular, unless it
> can return an array of connected media. I suspect these erroneous
> plurals are used in documentation and such so I don't want to
> mess around with things for no functional change.
>
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> drivers/net/usb/rndis_host.c | 6 ++--
> drivers/usb/gadget/f_rndis.c | 6 ++--
> drivers/usb/gadget/rndis.c | 8 ++--
> include/linux/rndis.h | 70
> ++++++++++++-----------------------------
> 4 files changed, 31 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> index f56b9f7..2dd47ec 100644
> --- a/drivers/net/usb/rndis_host.c
> +++ b/drivers/net/usb/rndis_host.c
> @@ -400,18 +400,18 @@ generic_rndis_bind(struct usbnet *dev, struct
> usb_interface *intf, int flags)
> 0, (void **) &phym, &reply_len);
> if (retval != 0 || !phym) {
> /* OID is optional so don't fail here. */
> - phym_unspec = cpu_to_le32(RNDIS_PHYSICAL_MEDIUM_UNSPECIFIED);
> + phym_unspec = cpu_to_le32(RNDIS_MEDIUM_UNSPECIFIED);
> phym = &phym_unspec;
> }
> if ((flags & FLAG_RNDIS_PHYM_WIRELESS) &&
> - *phym != cpu_to_le32(RNDIS_PHYSICAL_MEDIUM_WIRELESS_LAN)) {
> + *phym != cpu_to_le32(RNDIS_MEDIUM_WIRELESS_LAN)) {
This does not work.. RNDIS_PHYSICAL_MEDIUM_WIRELESS_LAN == 0x01 and
RNDIS_MEDIUM_WIRELESS_LAN == 0x09.
> netif_dbg(dev, probe, dev->net,
> "driver requires wireless physical medium, but device is not\n");
> retval = -ENODEV;
> goto halt_fail_and_release;
> }
> if ((flags & FLAG_RNDIS_PHYM_NOT_WIRELESS) &&
> - *phym == cpu_to_le32(RNDIS_PHYSICAL_MEDIUM_WIRELESS_LAN)) {
> + *phym == cpu_to_le32(RNDIS_MEDIUM_WIRELESS_LAN)) {
> netif_dbg(dev, probe, dev->net,
> "driver requires non-wireless physical medium, but device is
> wireless.\n");
> retval = -ENODEV;
> diff --git a/drivers/usb/gadget/f_rndis.c b/drivers/usb/gadget/f_rndis.c
> index 7b1cf18..c25d24e 100644
> --- a/drivers/usb/gadget/f_rndis.c
> +++ b/drivers/usb/gadget/f_rndis.c
> @@ -636,7 +636,7 @@ static void rndis_open(struct gether *geth)
>
> DBG(cdev, "%s\n", __func__);
>
> - rndis_set_param_medium(rndis->config, NDIS_MEDIUM_802_3,
> + rndis_set_param_medium(rndis->config, RNDIS_MEDIUM_802_3,
> bitrate(cdev->gadget) / 100);
> rndis_signal_connect(rndis->config);
> }
> @@ -647,7 +647,7 @@ static void rndis_close(struct gether *geth)
>
> DBG(geth->func.config->cdev, "%s\n", __func__);
>
> - rndis_set_param_medium(rndis->config, NDIS_MEDIUM_802_3, 0);
> + rndis_set_param_medium(rndis->config, RNDIS_MEDIUM_802_3, 0);
> rndis_signal_disconnect(rndis->config);
> }
>
> @@ -764,7 +764,7 @@ rndis_bind(struct usb_configuration *c, struct
> usb_function *f)
> goto fail;
> rndis->config = status;
>
> - rndis_set_param_medium(rndis->config, NDIS_MEDIUM_802_3, 0);
> + rndis_set_param_medium(rndis->config, RNDIS_MEDIUM_802_3, 0);
> rndis_set_host_mac(rndis->config, rndis->ethaddr);
>
> #if 0
> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
> index 79ed261..d9086ca 100644
> --- a/drivers/usb/gadget/rndis.c
> +++ b/drivers/usb/gadget/rndis.c
> @@ -252,7 +252,7 @@ static int gen_ndis_query_resp(int configNr, u32
> OID, u8 *buf,
> if (rndis_debug > 1)
> pr_debug("%s: RNDIS_OID_GEN_LINK_SPEED\n", __func__);
> if (rndis_per_dev_params[configNr].media_state
> - == NDIS_MEDIA_STATE_DISCONNECTED)
> + == RNDIS_MEDIA_STATE_DISCONNECTED)
> *outbuf = cpu_to_le32(0);
> else
> *outbuf = cpu_to_le32(
> @@ -758,7 +758,7 @@ static int rndis_indicate_status_msg(int
> configNr, u32 status)
> int rndis_signal_connect(int configNr)
> {
> rndis_per_dev_params[configNr].media_state
> - = NDIS_MEDIA_STATE_CONNECTED;
> + = RNDIS_MEDIA_STATE_CONNECTED;
> return rndis_indicate_status_msg(configNr,
> RNDIS_STATUS_MEDIA_CONNECT);
> }
> @@ -766,7 +766,7 @@ int rndis_signal_connect(int configNr)
> int rndis_signal_disconnect(int configNr)
> {
> rndis_per_dev_params[configNr].media_state
> - = NDIS_MEDIA_STATE_DISCONNECTED;
> + = RNDIS_MEDIA_STATE_DISCONNECTED;
> return rndis_indicate_status_msg(configNr,
> RNDIS_STATUS_MEDIA_DISCONNECT);
> }
> @@ -1173,7 +1173,7 @@ int rndis_init(void)
> rndis_per_dev_params[i].used = 0;
> rndis_per_dev_params[i].state = RNDIS_UNINITIALIZED;
> rndis_per_dev_params[i].media_state
> - = NDIS_MEDIA_STATE_DISCONNECTED;
> + = RNDIS_MEDIA_STATE_DISCONNECTED;
> INIT_LIST_HEAD(&(rndis_per_dev_params[i].resp_queue));
> }
>
> diff --git a/include/linux/rndis.h b/include/linux/rndis.h
> index 2e0b1bd..705dccc 100644
> --- a/include/linux/rndis.h
> +++ b/include/linux/rndis.h
> @@ -100,17 +100,27 @@
>
> #define RNDIS_STATUS_TOKEN_RING_OPEN_ERROR 0xC0011000
>
> -/* codes for RNDIS_OID_GEN_PHYSICAL_MEDIUM */
> -#define RNDIS_PHYSICAL_MEDIUM_UNSPECIFIED 0x00000000
> -#define RNDIS_PHYSICAL_MEDIUM_WIRELESS_LAN 0x00000001
> -#define RNDIS_PHYSICAL_MEDIUM_CABLE_MODEM 0x00000002
> -#define RNDIS_PHYSICAL_MEDIUM_PHONE_LINE 0x00000003
> -#define RNDIS_PHYSICAL_MEDIUM_POWER_LINE 0x00000004
> -#define RNDIS_PHYSICAL_MEDIUM_DSL 0x00000005
> -#define RNDIS_PHYSICAL_MEDIUM_FIBRE_CHANNEL 0x00000006
> -#define RNDIS_PHYSICAL_MEDIUM_1394 0x00000007
> -#define RNDIS_PHYSICAL_MEDIUM_WIRELESS_WAN 0x00000008
> -#define RNDIS_PHYSICAL_MEDIUM_MAX 0x00000009
> +/* Remote NDIS medium types. */
> +#define RNDIS_MEDIUM_UNSPECIFIED 0x00000000
> +#define RNDIS_MEDIUM_802_3 0x00000000
> +#define RNDIS_MEDIUM_802_5 0x00000001
> +#define RNDIS_MEDIUM_FDDI 0x00000002
> +#define RNDIS_MEDIUM_WAN 0x00000003
> +#define RNDIS_MEDIUM_LOCAL_TALK 0x00000004
> +#define RNDIS_MEDIUM_ARCNET_RAW 0x00000006
> +#define RNDIS_MEDIUM_ARCNET_878_2 0x00000007
> +#define RNDIS_MEDIUM_ATM 0x00000008
> +#define RNDIS_MEDIUM_WIRELESS_LAN 0x00000009
> +#define RNDIS_MEDIUM_IRDA 0x0000000A
> +#define RNDIS_MEDIUM_BPC 0x0000000B
> +#define RNDIS_MEDIUM_CO_WAN 0x0000000C
> +#define RNDIS_MEDIUM_1394 0x0000000D
NDIS medium type enumeration is not same as NDIS physical medium enumeration..
http://msdn.microsoft.com/en-us/library/windows/hardware/ff565910%28v=vs.85%29.aspx
http://msdn.microsoft.com/en-us/library/windows/hardware/ff569621%28v=vs.85%29.aspx
> +/* Not a real medium, defined as an upper-bound */
> +#define RNDIS_MEDIUM_MAX 0x0000000E
> +
> +/* Remote NDIS medium connection states. */
> +#define RNDIS_MEDIA_STATE_CONNECTED 0x00000000
> +#define RNDIS_MEDIA_STATE_DISCONNECTED 0x00000001
>
> /* packet filter bits used by RNDIS_OID_GEN_CURRENT_PACKET_FILTER */
> #define RNDIS_PACKET_TYPE_DIRECTED 0x00000001
> @@ -163,21 +173,6 @@
> #define NDIS_MINIPORT_SUPPORTS_CANCEL_SEND_PACKETS 0x00800000
> #define NDIS_MINIPORT_64BITS_DMA 0x01000000
>
> -#define NDIS_MEDIUM_802_3 0x00000000
> -#define NDIS_MEDIUM_802_5 0x00000001
> -#define NDIS_MEDIUM_FDDI 0x00000002
> -#define NDIS_MEDIUM_WAN 0x00000003
> -#define NDIS_MEDIUM_LOCAL_TALK 0x00000004
> -#define NDIS_MEDIUM_DIX 0x00000005
> -#define NDIS_MEDIUM_ARCENT_RAW 0x00000006
> -#define NDIS_MEDIUM_ARCENT_878_2 0x00000007
> -#define NDIS_MEDIUM_ATM 0x00000008
> -#define NDIS_MEDIUM_WIRELESS_LAN 0x00000009
> -#define NDIS_MEDIUM_IRDA 0x0000000A
> -#define NDIS_MEDIUM_BPC 0x0000000B
> -#define NDIS_MEDIUM_CO_WAN 0x0000000C
> -#define NDIS_MEDIUM_1394 0x0000000D
> -
> #define NDIS_PACKET_TYPE_DIRECTED 0x00000001
> #define NDIS_PACKET_TYPE_MULTICAST 0x00000002
> #define NDIS_PACKET_TYPE_ALL_MULTICAST 0x00000004
> @@ -191,9 +186,6 @@
> #define NDIS_PACKET_TYPE_FUNCTIONAL 0x00000400
> #define NDIS_PACKET_TYPE_MAC_FRAME 0x00000800
>
> -#define NDIS_MEDIA_STATE_CONNECTED 0x00000000
> -#define NDIS_MEDIA_STATE_DISCONNECTED 0x00000001
> -
> #define NDIS_MAC_OPTION_COPY_LOOKAHEAD_DATA 0x00000001
> #define NDIS_MAC_OPTION_RECEIVE_SERIALIZED 0x00000002
> #define NDIS_MAC_OPTION_TRANSFERS_NOT_PEND 0x00000004
> @@ -421,23 +413,3 @@
> * driver and remote device, if necessary.
> */
> #define REMOTE_NDIS_BUS_MSG 0xff000001
> -
> -/* Remote NDIS medium types. */
> -#define RNDIS_MEDIUM_802_3 0x00000000
> -#define RNDIS_MEDIUM_802_5 0x00000001
> -#define RNDIS_MEDIUM_FDDI 0x00000002
> -#define RNDIS_MEDIUM_WAN 0x00000003
> -#define RNDIS_MEDIUM_LOCAL_TALK 0x00000004
> -#define RNDIS_MEDIUM_ARCNET_RAW 0x00000006
> -#define RNDIS_MEDIUM_ARCNET_878_2 0x00000007
> -#define RNDIS_MEDIUM_ATM 0x00000008
> -#define RNDIS_MEDIUM_WIRELESS_WAN 0x00000009
> -#define RNDIS_MEDIUM_IRDA 0x0000000a
> -#define RNDIS_MEDIUM_CO_WAN 0x0000000b
> -/* Not a real medium, defined as an upper-bound */
> -#define RNDIS_MEDIUM_MAX 0x0000000d
> -
> -
> -/* Remote NDIS medium connection states. */
> -#define RNDIS_MEDIA_STATE_CONNECTED 0x00000000
> -#define RNDIS_MEDIA_STATE_DISCONNECTED 0x00000001
> --
> 1.7.7.6
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] phonet: Sort out initiailziation and cleanup code.
From: Rémi Denis-Courmont @ 2012-04-10 6:39 UTC (permalink / raw)
To: ext Eric W. Biederman
Cc: David Miller, Eric Dumazet, Eric Van Hensbergen, Dave Jones,
linux-kernel, netdev, Sasha Levin
In-Reply-To: <m162dctjbo.fsf_-_@fess.ebiederm.org>
Le vendredi 6 avril 2012 18:35:39 ext Eric W. Biederman a écrit :
> Recently an oops was reported in phonet if there was a failure during
> network namespace creation.
>
> [ 163.733755] ------------[ cut here ]------------
> [ 163.734501] kernel BUG at include/net/netns/generic.h:45!
> [ 163.734501] invalid opcode: 0000 [#1] PREEMPT SMP
> [ 163.734501] CPU 2
> [ 163.734501] Pid: 19145, comm: trinity Tainted: G W
> 3.4.0-rc1-next-20120405-sasha-dirty #57 [ 163.734501] RIP:
> 0010:[<ffffffff824d6062>] [<ffffffff824d6062>] phonet_pernet+0x182/0x1a0 [
> 163.734501] RSP: 0018:ffff8800674d5ca8 EFLAGS: 00010246
> [ 163.734501] RAX: 000000003fffffff RBX: 0000000000000000 RCX:
> ffff8800678c88d8 [ 163.734501] RDX: 00000000003f4000 RSI: ffff8800678c8910
> RDI: 0000000000000282 [ 163.734501] RBP: ffff8800674d5cc8 R08:
> 0000000000000000 R09: 0000000000000000 [ 163.734501] R10: 0000000000000000
> R11: 0000000000000000 R12: ffff880068bec920 [ 163.734501] R13:
> ffffffff836b90c0 R14: 0000000000000000 R15: 0000000000000000 [ 163.734501]
> FS: 00007f055e8de700(0000) GS:ffff88007d000000(0000)
> knlGS:0000000000000000 [ 163.734501] CS: 0010 DS: 0000 ES: 0000 CR0:
> 000000008005003b
> [ 163.734501] CR2: 00007f055e6bb518 CR3: 0000000070c16000 CR4:
> 00000000000406e0 [ 163.734501] DR0: 0000000000000000 DR1: 0000000000000000
> DR2: 0000000000000000 [ 163.734501] DR3: 0000000000000000 DR6:
> 00000000ffff0ff0 DR7: 0000000000000400 [ 163.734501] Process trinity (pid:
> 19145, threadinfo ffff8800674d4000, task ffff8800678c8000) [ 163.734501]
> Stack:
> [ 163.734501] ffffffff824d5f00 ffffffff810e2ec1 ffff880067ae0000
> 00000000ffffffd4 [ 163.734501] ffff8800674d5cf8 ffffffff824d667a
> ffff880067ae0000 00000000ffffffd4 [ 163.734501] ffffffff836b90c0
> 0000000000000000 ffff8800674d5d18 ffffffff824d707d [ 163.734501] Call
> Trace:
> [ 163.734501] [<ffffffff824d5f00>] ? phonet_pernet+0x20/0x1a0
> [ 163.734501] [<ffffffff810e2ec1>] ? get_parent_ip+0x11/0x50
> [ 163.734501] [<ffffffff824d667a>] phonet_device_destroy+0x1a/0x100
> [ 163.734501] [<ffffffff824d707d>] phonet_device_notify+0x3d/0x50
> [ 163.734501] [<ffffffff810dd96e>] notifier_call_chain+0xee/0x130
> [ 163.734501] [<ffffffff810dd9d1>] raw_notifier_call_chain+0x11/0x20
> [ 163.734501] [<ffffffff821cce12>] call_netdevice_notifiers+0x52/0x60
> [ 163.734501] [<ffffffff821cd235>] rollback_registered_many+0x185/0x270
> [ 163.734501] [<ffffffff821cd334>] unregister_netdevice_many+0x14/0x60
> [ 163.734501] [<ffffffff823123e3>] ipip_exit_net+0x1b3/0x1d0
> [ 163.734501] [<ffffffff82312230>] ? ipip_rcv+0x420/0x420
> [ 163.734501] [<ffffffff821c8515>] ops_exit_list+0x35/0x70
> [ 163.734501] [<ffffffff821c911b>] setup_net+0xab/0xe0
> [ 163.734501] [<ffffffff821c9416>] copy_net_ns+0x76/0x100
> [ 163.734501] [<ffffffff810dc92b>] create_new_namespaces+0xfb/0x190
> [ 163.734501] [<ffffffff810dca21>] unshare_nsproxy_namespaces+0x61/0x80
> [ 163.734501] [<ffffffff810afd1f>] sys_unshare+0xff/0x290
> [ 163.734501] [<ffffffff8187622e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [ 163.734501] [<ffffffff82665539>] system_call_fastpath+0x16/0x1b
> [ 163.734501] Code: e0 c3 fe 66 0f 1f 44 00 00 48 c7 c2 40 60 4d 82 be 01
> 00 00 00 48 c7 c7 80 d1 23 83 e8 48 2a c4 fe e8 73 06 c8 fe 48 85 db 75 0e
> <0f> 0b 0f 1f 40 00 eb fe 66 0f 1f 44 00 00 48 83 c4 10 48 89 d8 [
> 163.734501] RIP [<ffffffff824d6062>] phonet_pernet+0x182/0x1a0 [
> 163.734501] RSP <ffff8800674d5ca8>
> [ 163.861289] ---[ end trace fb5615826c548066 ]---
>
> After investigation it turns out there were two issues.
> 1) Phonet was not implementing network devices but was using
> register_pernet_device instead of register_pernet_subsys.
>
> This was allowing there to be cases when phonenet was not initialized and
> the phonet net_generic was not set for a network namespace when network
> device events were being reported on the netdevice_notifier for a network
> namespace leading to the oops above.
>
> 2) phonet_exit_net was implementing a confusing and special case of handling
> all network devices from going away that it was hard to see was correct,
> and would only occur when the phonet module was removed.
>
> Now that unregister_netdevice_notifier has been modified to synthesize
> unregistration events for the network devices that are extant when called
> this confusing special case in phonet_exit_net is no longer needed.
>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Acked-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> ---
> net/phonet/pn_dev.c | 21 ++-------------------
> 1 files changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/net/phonet/pn_dev.c b/net/phonet/pn_dev.c
> index 9b9a85e..bf5cf69 100644
> --- a/net/phonet/pn_dev.c
> +++ b/net/phonet/pn_dev.c
> @@ -331,23 +331,6 @@ static int __net_init phonet_init_net(struct net *net)
>
> static void __net_exit phonet_exit_net(struct net *net)
> {
> - struct phonet_net *pnn = phonet_pernet(net);
> - struct net_device *dev;
> - unsigned i;
> -
> - rtnl_lock();
> - for_each_netdev(net, dev)
> - phonet_device_destroy(dev);
> -
> - for (i = 0; i < 64; i++) {
> - dev = pnn->routes.table[i];
> - if (dev) {
> - rtm_phonet_notify(RTM_DELROUTE, dev, i);
> - dev_put(dev);
> - }
> - }
> - rtnl_unlock();
> -
> proc_net_remove(net, "phonet");
> }
>
> @@ -361,7 +344,7 @@ static struct pernet_operations phonet_net_ops = {
> /* Initialize Phonet devices list */
> int __init phonet_device_init(void)
> {
> - int err = register_pernet_device(&phonet_net_ops);
> + int err = register_pernet_subsys(&phonet_net_ops);
> if (err)
> return err;
>
> @@ -377,7 +360,7 @@ void phonet_device_exit(void)
> {
> rtnl_unregister_all(PF_PHONET);
> unregister_netdevice_notifier(&phonet_device_notifier);
> - unregister_pernet_device(&phonet_net_ops);
> + unregister_pernet_subsys(&phonet_net_ops);
> proc_net_remove(&init_net, "pnresource");
> }
--
Rémi Denis-Courmont
http://www.remlab.net/
^ permalink raw reply
* Re: [PATCH] gianfar: add missing include
From: Michael Neuling @ 2012-04-10 6:41 UTC (permalink / raw)
To: Richard Cochran; +Cc: David S. Miller, linuxppc-dev, netdev, linux-next
In-Reply-To: <20120410063641.GA10025@localhost.localdomain>
Richard Cochran <richardcochran@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 02:18:47PM +1000, Michael Neuling wrote:
> > This is because of a missing include file from:
> > 6663628 gianfar: Support the get_ts_info ethtool method.
> >
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>
> I did a poor job testing the non-x86 stuff in this series. Thanks for
> the fix.
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
Looks like davem fixed it before my post anyway...
http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commitdiff;h=65a85a8d4dcabe95587ab61bece38b8d58174043
This is in next-20120410.
Sorry for the noise.
Mikey
^ permalink raw reply
* Re: [PATCH 2/2] qlcnic: Remove redundant NULL check on kfree().
From: Dan Carpenter @ 2012-04-10 7:49 UTC (permalink / raw)
To: santosh nayak
Cc: anirban.chakraborty, rajesh.borundia, sony.chacko, linux-driver,
netdev, kernel-janitors
In-Reply-To: <1333951879-3230-1-git-send-email-santoshprasadnayak@gmail.com>
On Mon, Apr 09, 2012 at 11:41:19AM +0530, santosh nayak wrote:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>
> kfree() checks for NULL before freeing the memory.
> Remove redundant NULL check.
> This is just a clean up and also looks good from performance point of view.
I doubt it has any impact on performance.
[snip]
> static void qlcnic_free_lb_filters_mem(struct qlcnic_adapter *adapter)
> {
> - if (adapter->fhash.fmax && adapter->fhash.fhead)
> - kfree(adapter->fhash.fhead);
> -
> + kfree(adapter->fhash.fhead);
This changes how the code works. That's very sloppy to not notice
that. If you did notice it, it should be justified in the
changelog. :(
> adapter->fhash.fhead = NULL;
> adapter->fhash.fmax = 0;
> }
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Eric Dumazet @ 2012-04-10 7:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Michał Mirosław, Ben Hutchings,
Herbert Xu
In-Reply-To: <20120408171323.GA16012@redhat.com>
On Sun, 2012-04-08 at 20:13 +0300, Michael S. Tsirkin wrote:
> commit 0110d6f22f392f976e84ab49da1b42f85b64a3c5
> tun: orphan an skb on tx
> Fixed a configuration where skbs get queued
> at the tun device forever, blocking senders.
>
> However this fix isn't waterproof:
> userspace can control whether the interface
> is stopped, and if it is, packets
> get queued in the qdisc, again potentially forever.
>
> Complete the fix by setting a private flag and orphaning
> at the qdisc level.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/tun.c | 3 +++
> include/linux/if.h | 1 +
> net/core/dev.c | 5 +++++
> net/sched/sch_generic.c | 5 +++++
> 4 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index bb8c72c..15c5bb8 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -535,6 +535,9 @@ static void tun_net_init(struct net_device *dev)
> dev->tx_queue_len = TUN_READQ_SIZE; /* We prefer our own queue length */
> break;
> }
> + /* Once queue becomes full, we stop tx until userspace
> + * dequeues some packets, that is potentially forever. */
> + dev->priv_flags |= IFF_TX_CAN_STALL;
> }
>
> /* Character device part */
> diff --git a/include/linux/if.h b/include/linux/if.h
> index f995c66..dd2c7f7 100644
> --- a/include/linux/if.h
> +++ b/include/linux/if.h
> @@ -81,6 +81,7 @@
> #define IFF_UNICAST_FLT 0x20000 /* Supports unicast filtering */
> #define IFF_TEAM_PORT 0x40000 /* device used as team port */
> #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */
> +#define IFF_TX_CAN_STALL 0x100000 /* Device can stop tx forever */
>
>
> #define IF_GET_IFACE 0x0001 /* for querying only */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5d59155..e812706 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2516,6 +2516,11 @@ int dev_queue_xmit(struct sk_buff *skb)
> struct Qdisc *q;
> int rc = -ENOMEM;
>
> + /* Orphan the skb - required if we might hang on to it
> + * for indefinite time. */
> + if (dev->priv_flags & IFF_TX_CAN_STALL)
> + skb_orphan(skb);
> +
> /* Disable soft irqs for various locks below. Also
> * stops preemption for RCU.
> */
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 67fc573..27883d1 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -120,6 +120,11 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
> /* And release qdisc */
> spin_unlock(root_lock);
>
> + /* Orphan the skb - required if we might hang on to it
> + * for indefinite time. */
> + if (dev->priv_flags & IFF_TX_CAN_STALL)
> + skb_orphan(skb);
> +
> HARD_TX_LOCK(dev, txq, smp_processor_id());
> if (!netif_xmit_frozen_or_stopped(txq))
> ret = dev_hard_start_xmit(skb, dev, txq);
This slows down the core fastpath for a very specific use.
In your case I would just not use qdisc at all, like other virtual
devices.
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index bb8c72c..fd8c7f0 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -396,7 +396,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
sk_filter(tun->socket.sk, skb))
goto drop;
- if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= dev->tx_queue_len) {
+ if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= TUN_READQ_SIZE) {
if (!(tun->flags & TUN_ONE_QUEUE)) {
/* Normal queueing mode. */
/* Packet scheduler handles dropping of further packets. */
@@ -521,7 +521,7 @@ static void tun_net_init(struct net_device *dev)
/* Zero header length */
dev->type = ARPHRD_NONE;
dev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
- dev->tx_queue_len = TUN_READQ_SIZE; /* We prefer our own queue length */
+ dev->tx_queue_len = 0;
break;
case TUN_TAP_DEV:
@@ -532,7 +532,7 @@ static void tun_net_init(struct net_device *dev)
eth_hw_addr_random(dev);
- dev->tx_queue_len = TUN_READQ_SIZE; /* We prefer our own queue length */
+ dev->tx_queue_len = 0;
break;
}
}
^ permalink raw reply related
* Re: [net-next PATCH v1 2/7] net: addr_list: add exclusive dev_uc_add and dev_mc_add
From: Michael S. Tsirkin @ 2012-04-10 8:03 UTC (permalink / raw)
To: John Fastabend
Cc: roprabhu, stephen.hemminger, davem, hadi, bhutchings,
jeffrey.t.kirsher, netdev, gregory.v.rose, krkumar2, sri
In-Reply-To: <20120409220023.3288.59939.stgit@jf-dev1-dcblab>
On Mon, Apr 09, 2012 at 03:00:23PM -0700, John Fastabend wrote:
> This adds a dev_uc_add_excl() and dev_mc_add_excl() calls
> similar to the original dev_{uc|mc}_add() except it sets
> the global bit and returns -EEXIST for duplicat entires.
>
> This is useful for drivers that support SR-IOV, macvlan
> devices and any other devices that need to manage the
> unicast and multicast lists.
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>
> include/linux/netdevice.h | 2 +
> net/core/dev_addr_lists.c | 97 ++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 83 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 05822e5..b68a326 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2572,6 +2572,7 @@ extern int dev_addr_init(struct net_device *dev);
>
> /* Functions used for unicast addresses handling */
> extern int dev_uc_add(struct net_device *dev, unsigned char *addr);
> +extern int dev_uc_add_excl(struct net_device *dev, unsigned char *addr);
> extern int dev_uc_del(struct net_device *dev, unsigned char *addr);
> extern int dev_uc_sync(struct net_device *to, struct net_device *from);
> extern void dev_uc_unsync(struct net_device *to, struct net_device *from);
> @@ -2581,6 +2582,7 @@ extern void dev_uc_init(struct net_device *dev);
> /* Functions used for multicast addresses handling */
> extern int dev_mc_add(struct net_device *dev, unsigned char *addr);
> extern int dev_mc_add_global(struct net_device *dev, unsigned char *addr);
> +extern int dev_mc_add_excl(struct net_device *dev, unsigned char *addr);
> extern int dev_mc_del(struct net_device *dev, unsigned char *addr);
> extern int dev_mc_del_global(struct net_device *dev, unsigned char *addr);
> extern int dev_mc_sync(struct net_device *to, struct net_device *from);
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 29c07fe..5405e28 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -21,12 +21,35 @@
> * General list handling functions
> */
>
> +static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
> + unsigned char *addr, int addr_len,
> + unsigned char addr_type, bool global)
> +{
> + struct netdev_hw_addr *ha;
> + int alloc_size;
> +
> + alloc_size = sizeof(*ha);
> + if (alloc_size < L1_CACHE_BYTES)
> + alloc_size = L1_CACHE_BYTES;
> + ha = kmalloc(alloc_size, GFP_ATOMIC);
> + if (!ha)
> + return -ENOMEM;
> + memcpy(ha->addr, addr, addr_len);
> + ha->type = addr_type;
> + ha->refcount = 1;
> + ha->global_use = global;
> + ha->synced = false;
> + list_add_tail_rcu(&ha->list, &list->list);
> + list->count++;
> +
> + return 0;
> +}
> +
> static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
> unsigned char *addr, int addr_len,
> unsigned char addr_type, bool global)
> {
> struct netdev_hw_addr *ha;
> - int alloc_size;
>
> if (addr_len > MAX_ADDR_LEN)
> return -EINVAL;
> @@ -46,21 +69,7 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
> }
> }
>
> -
> - alloc_size = sizeof(*ha);
> - if (alloc_size < L1_CACHE_BYTES)
> - alloc_size = L1_CACHE_BYTES;
> - ha = kmalloc(alloc_size, GFP_ATOMIC);
> - if (!ha)
> - return -ENOMEM;
> - memcpy(ha->addr, addr, addr_len);
> - ha->type = addr_type;
> - ha->refcount = 1;
> - ha->global_use = global;
> - ha->synced = false;
> - list_add_tail_rcu(&ha->list, &list->list);
> - list->count++;
> - return 0;
> + return __hw_addr_create_ex(list, addr, addr_len, addr_type, global);
> }
>
> static int __hw_addr_add(struct netdev_hw_addr_list *list, unsigned char *addr,
> @@ -377,6 +386,34 @@ EXPORT_SYMBOL(dev_addr_del_multiple);
> */
>
> /**
> + * dev_uc_add_excl - Add a global secondary unicast address
> + * @dev: device
> + * @addr: address to add
> + */
> +int dev_uc_add_excl(struct net_device *dev, unsigned char *addr)
> +{
> + struct netdev_hw_addr *ha;
> + int err;
> +
> + netif_addr_lock_bh(dev);
> + list_for_each_entry(ha, &dev->uc.list, list) {
> + if (!memcmp(ha->addr, addr, dev->addr_len) &&
> + ha->type == NETDEV_HW_ADDR_T_UNICAST) {
> + err = -EEXIST;
> + goto out;
> + }
> + }
> + err = __hw_addr_create_ex(&dev->uc, addr, dev->addr_len,
> + NETDEV_HW_ADDR_T_UNICAST, true);
> + if (!err)
> + __dev_set_rx_mode(dev);
> +out:
> + netif_addr_unlock_bh(dev);
> + return err;
> +}
> +EXPORT_SYMBOL(dev_uc_add_excl);
> +
> +/**
> * dev_uc_add - Add a secondary unicast address
> * @dev: device
> * @addr: address to add
> @@ -501,6 +538,34 @@ EXPORT_SYMBOL(dev_uc_init);
> * Multicast list handling functions
> */
>
> +/**
> + * dev_mc_add_excl - Add a global secondary multicast address
> + * @dev: device
> + * @addr: address to add
> + */
> +int dev_mc_add_excl(struct net_device *dev, unsigned char *addr)
> +{
> + struct netdev_hw_addr *ha;
> + int err;
> +
> + netif_addr_lock_bh(dev);
> + list_for_each_entry(ha, &dev->mc.list, list) {
> + if (!memcmp(ha->addr, addr, dev->addr_len) &&
> + ha->type == NETDEV_HW_ADDR_T_UNICAST) {
> + err = -EEXIST;
> + goto out;
> + }
> + }
> + err = __hw_addr_create_ex(&dev->mc, addr, dev->addr_len,
> + NETDEV_HW_ADDR_T_UNICAST, true);
> + if (!err)
> + __dev_set_rx_mode(dev);
> +out:
> + netif_addr_unlock_bh(dev);
> + return err;
> +}
> +EXPORT_SYMBOL(dev_mc_add_excl);
> +
Why is it a mistake to have duplicate multicast addresses?
For example macvlan floods all multicasts so it's fine,
and assuming it didn't, it might still be fine to have to
macvlans get the same multicast, no?
> static int __dev_mc_add(struct net_device *dev, unsigned char *addr,
> bool global)
> {
^ permalink raw reply
* Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode
From: Michael S. Tsirkin @ 2012-04-10 8:09 UTC (permalink / raw)
To: John Fastabend
Cc: roprabhu, stephen.hemminger, davem, hadi, bhutchings,
jeffrey.t.kirsher, netdev, gregory.v.rose, krkumar2, sri
In-Reply-To: <20120409220053.3288.40867.stgit@jf-dev1-dcblab>
On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
> this mode acts the same as the original passthru mode _except_
> it does not set promiscuous mode on the lowerdev. Because the
> lowerdev is not put in promiscuous mode any unicast or multicast
> addresses the device should receive must be explicitely added
> with the FDB bridge ops. In many use cases the management stack
> will know the mac addresses needed (maybe negotiated via EVB/VDP)
> or may require only receiving known "good" mac addresses. This
> mode with the FDB ops supports this usage model.
Looks good to me. Some questions below:
> This patch is a result of Roopa Prabhu's work. Follow up
> patches are needed for VEPA and VEB macvlan modes.
And bridge too?
Also, my understanding is that other modes won't need a flag
like this since they don't put the device in promisc mode initially,
so no assumptions are broken if we require all addresses
to be declared, right?
A final question: I think we'll later add a macvlan mode
that does not flood all multicasts. This would change behaviour
in an incompatible way so we'll probably need yet another
flag. Would it make sense to combine this functionality
with nopromisc so we have less modes to support?
> CC: Roopa Prabhu <roprabhu@cisco.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>
> drivers/net/macvlan.c | 60 ++++++++++++++++++++++++++++++++++++++++++-----
> include/linux/if_link.h | 1 +
> 2 files changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index b17fc90..9892d8d 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -181,6 +181,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
> MACVLAN_MODE_PRIVATE |
> MACVLAN_MODE_VEPA |
> MACVLAN_MODE_PASSTHRU|
> + MACVLAN_MODE_PASSTHRU_NOPROMISC |
> MACVLAN_MODE_BRIDGE);
> else if (src->mode == MACVLAN_MODE_VEPA)
> /* flood to everyone except source */
> @@ -312,7 +313,8 @@ static int macvlan_open(struct net_device *dev)
> int err;
>
> if (vlan->port->passthru) {
> - dev_set_promiscuity(lowerdev, 1);
> + if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> + dev_set_promiscuity(lowerdev, 1);
> goto hash_add;
> }
>
> @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
> struct macvlan_dev *vlan = netdev_priv(dev);
> struct net_device *lowerdev = vlan->lowerdev;
>
> + dev_uc_unsync(lowerdev, dev);
> + dev_mc_unsync(lowerdev, dev);
> +
> if (vlan->port->passthru) {
> - dev_set_promiscuity(lowerdev, -1);
> + if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> + dev_set_promiscuity(lowerdev, 1);
> goto hash_del;
> }
>
> - dev_mc_unsync(lowerdev, dev);
> if (dev->flags & IFF_ALLMULTI)
> dev_set_allmulti(lowerdev, -1);
>
> @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
> dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
> }
>
> -static void macvlan_set_multicast_list(struct net_device *dev)
> +static void macvlan_set_mac_lists(struct net_device *dev)
> {
> struct macvlan_dev *vlan = netdev_priv(dev);
>
> + dev_uc_sync(vlan->lowerdev, dev);
> dev_mc_sync(vlan->lowerdev, dev);
> }
>
> @@ -542,6 +548,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev,
> return 0;
> }
>
> +static int macvlan_fdb_add(struct ndmsg *ndm,
> + struct net_device *dev,
> + unsigned char *addr,
> + u16 flags)
> +{
> + struct macvlan_dev *vlan = netdev_priv(dev);
> + int err = -EINVAL;
> +
> + if (!vlan->port->passthru)
> + return -EOPNOTSUPP;
> +
> + if (is_unicast_ether_addr(addr))
> + err = dev_uc_add_excl(dev, addr);
> + else if (is_multicast_ether_addr(addr))
> + err = dev_mc_add_excl(dev, addr);
> +
> + return err;
> +}
> +
> +static int macvlan_fdb_del(struct ndmsg *ndm,
> + struct net_device *dev,
> + unsigned char *addr)
> +{
> + struct macvlan_dev *vlan = netdev_priv(dev);
> + int err = -EINVAL;
> +
> + if (!vlan->port->passthru)
> + return -EOPNOTSUPP;
> +
> + if (is_unicast_ether_addr(addr))
> + err = dev_uc_del(dev, addr);
> + else if (is_multicast_ether_addr(addr))
> + err = dev_mc_del(dev, addr);
> +
> + return err;
> +}
> +
> static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
> struct ethtool_drvinfo *drvinfo)
> {
> @@ -572,11 +615,14 @@ static const struct net_device_ops macvlan_netdev_ops = {
> .ndo_change_mtu = macvlan_change_mtu,
> .ndo_change_rx_flags = macvlan_change_rx_flags,
> .ndo_set_mac_address = macvlan_set_mac_address,
> - .ndo_set_rx_mode = macvlan_set_multicast_list,
> + .ndo_set_rx_mode = macvlan_set_mac_lists,
> .ndo_get_stats64 = macvlan_dev_get_stats64,
> .ndo_validate_addr = eth_validate_addr,
> .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid,
> .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid,
> + .ndo_fdb_add = macvlan_fdb_add,
> + .ndo_fdb_del = macvlan_fdb_del,
> + .ndo_fdb_dump = ndo_dflt_fdb_dump,
> };
>
> void macvlan_common_setup(struct net_device *dev)
> @@ -648,6 +694,7 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
> case MACVLAN_MODE_VEPA:
> case MACVLAN_MODE_BRIDGE:
> case MACVLAN_MODE_PASSTHRU:
> + case MACVLAN_MODE_PASSTHRU_NOPROMISC:
> break;
> default:
> return -EINVAL;
> @@ -711,7 +758,8 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
> if (data && data[IFLA_MACVLAN_MODE])
> vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
>
> - if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> + if ((vlan->mode == MACVLAN_MODE_PASSTHRU) ||
> + (vlan->mode == MACVLAN_MODE_PASSTHRU_NOPROMISC)) {
> if (port->count)
> return -EINVAL;
> port->passthru = true;
> diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> index 2f4fa93..db67b9d 100644
> --- a/include/linux/if_link.h
> +++ b/include/linux/if_link.h
> @@ -265,6 +265,7 @@ enum macvlan_mode {
> MACVLAN_MODE_VEPA = 2, /* talk to other ports through ext bridge */
> MACVLAN_MODE_BRIDGE = 4, /* talk to bridge ports directly */
> MACVLAN_MODE_PASSTHRU = 8,/* take over the underlying device */
> + MACVLAN_MODE_PASSTHRU_NOPROMISC = 16, /* passthru without promisc */
> };
>
> /* SR-IOV virtual function management section */
^ permalink raw reply
* net: more accurate skb truesize - regression on Microblaze
From: Michal Simek @ 2012-04-10 8:10 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, John Williams, David Miller
Hi Eric,
I have spent some time to investigate what it is causing regression on our network bechmark
and I have identified that your patch "net: more accurate skb truesize" is causing that.
I see regression especially on incoming tcp (RX) connection which is 20%. On TX I see regression till 5%.
In general microblaze systems are pretty sensitive for memory usage and working with it.
Increasing packet sizes has big impact on performance.
I was surprised that regression is so high.
From microblaze point of view is more important to remove that performance regression
than having more accurate memory accounting. 20% performance regression is simple so high
for that.
Isn't there any other way to doing better memory accounting?
Or is there any workaround which would be possible to use?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode
From: Michael S. Tsirkin @ 2012-04-10 8:14 UTC (permalink / raw)
To: John Fastabend
Cc: roprabhu, stephen.hemminger, davem, hadi, bhutchings,
jeffrey.t.kirsher, netdev, gregory.v.rose, krkumar2, sri
In-Reply-To: <20120410080916.GB26540@redhat.com>
On Tue, Apr 10, 2012 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
> > This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
> > this mode acts the same as the original passthru mode _except_
> > it does not set promiscuous mode on the lowerdev. Because the
> > lowerdev is not put in promiscuous mode any unicast or multicast
> > addresses the device should receive must be explicitely added
> > with the FDB bridge ops. In many use cases the management stack
> > will know the mac addresses needed (maybe negotiated via EVB/VDP)
> > or may require only receiving known "good" mac addresses. This
> > mode with the FDB ops supports this usage model.
>
>
> Looks good to me. Some questions below:
>
> > This patch is a result of Roopa Prabhu's work. Follow up
> > patches are needed for VEPA and VEB macvlan modes.
>
> And bridge too?
>
> Also, my understanding is that other modes won't need a flag
> like this since they don't put the device in promisc mode initially,
> so no assumptions are broken if we require all addresses
> to be declared, right?
>
> A final question: I think we'll later add a macvlan mode
> that does not flood all multicasts. This would change behaviour
> in an incompatible way so we'll probably need yet another
> flag. Would it make sense to combine this functionality
> with nopromisc so we have less modes to support?
One other question I forgot:
> > CC: Roopa Prabhu <roprabhu@cisco.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> > ---
> >
> > drivers/net/macvlan.c | 60 ++++++++++++++++++++++++++++++++++++++++++-----
> > include/linux/if_link.h | 1 +
> > 2 files changed, 55 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> > index b17fc90..9892d8d 100644
> > --- a/drivers/net/macvlan.c
> > +++ b/drivers/net/macvlan.c
> > @@ -181,6 +181,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
> > MACVLAN_MODE_PRIVATE |
> > MACVLAN_MODE_VEPA |
> > MACVLAN_MODE_PASSTHRU|
> > + MACVLAN_MODE_PASSTHRU_NOPROMISC |
> > MACVLAN_MODE_BRIDGE);
> > else if (src->mode == MACVLAN_MODE_VEPA)
> > /* flood to everyone except source */
> > @@ -312,7 +313,8 @@ static int macvlan_open(struct net_device *dev)
> > int err;
> >
> > if (vlan->port->passthru) {
> > - dev_set_promiscuity(lowerdev, 1);
> > + if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> > + dev_set_promiscuity(lowerdev, 1);
> > goto hash_add;
> > }
> >
> > @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
> > struct macvlan_dev *vlan = netdev_priv(dev);
> > struct net_device *lowerdev = vlan->lowerdev;
> >
> > + dev_uc_unsync(lowerdev, dev);
> > + dev_mc_unsync(lowerdev, dev);
> > +
> > if (vlan->port->passthru) {
> > - dev_set_promiscuity(lowerdev, -1);
> > + if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> > + dev_set_promiscuity(lowerdev, 1);
> > goto hash_del;
> > }
> >
> > - dev_mc_unsync(lowerdev, dev);
> > if (dev->flags & IFF_ALLMULTI)
> > dev_set_allmulti(lowerdev, -1);
> >
> > @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
> > dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
In the new mode, do we want to have promisc on lowerdev follow whatever
is set on the macvlan, like we do for allmulti?
I'm not sure at this point - what do others think?
> > }
> >
> > -static void macvlan_set_multicast_list(struct net_device *dev)
> > +static void macvlan_set_mac_lists(struct net_device *dev)
> > {
> > struct macvlan_dev *vlan = netdev_priv(dev);
> >
> > + dev_uc_sync(vlan->lowerdev, dev);
> > dev_mc_sync(vlan->lowerdev, dev);
> > }
> >
> > @@ -542,6 +548,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev,
> > return 0;
> > }
> >
> > +static int macvlan_fdb_add(struct ndmsg *ndm,
> > + struct net_device *dev,
> > + unsigned char *addr,
> > + u16 flags)
> > +{
> > + struct macvlan_dev *vlan = netdev_priv(dev);
> > + int err = -EINVAL;
> > +
> > + if (!vlan->port->passthru)
> > + return -EOPNOTSUPP;
> > +
> > + if (is_unicast_ether_addr(addr))
> > + err = dev_uc_add_excl(dev, addr);
> > + else if (is_multicast_ether_addr(addr))
> > + err = dev_mc_add_excl(dev, addr);
> > +
> > + return err;
> > +}
> > +
> > +static int macvlan_fdb_del(struct ndmsg *ndm,
> > + struct net_device *dev,
> > + unsigned char *addr)
> > +{
> > + struct macvlan_dev *vlan = netdev_priv(dev);
> > + int err = -EINVAL;
> > +
> > + if (!vlan->port->passthru)
> > + return -EOPNOTSUPP;
> > +
> > + if (is_unicast_ether_addr(addr))
> > + err = dev_uc_del(dev, addr);
> > + else if (is_multicast_ether_addr(addr))
> > + err = dev_mc_del(dev, addr);
> > +
> > + return err;
> > +}
> > +
> > static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
> > struct ethtool_drvinfo *drvinfo)
> > {
> > @@ -572,11 +615,14 @@ static const struct net_device_ops macvlan_netdev_ops = {
> > .ndo_change_mtu = macvlan_change_mtu,
> > .ndo_change_rx_flags = macvlan_change_rx_flags,
> > .ndo_set_mac_address = macvlan_set_mac_address,
> > - .ndo_set_rx_mode = macvlan_set_multicast_list,
> > + .ndo_set_rx_mode = macvlan_set_mac_lists,
> > .ndo_get_stats64 = macvlan_dev_get_stats64,
> > .ndo_validate_addr = eth_validate_addr,
> > .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid,
> > .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid,
> > + .ndo_fdb_add = macvlan_fdb_add,
> > + .ndo_fdb_del = macvlan_fdb_del,
> > + .ndo_fdb_dump = ndo_dflt_fdb_dump,
> > };
> >
> > void macvlan_common_setup(struct net_device *dev)
> > @@ -648,6 +694,7 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
> > case MACVLAN_MODE_VEPA:
> > case MACVLAN_MODE_BRIDGE:
> > case MACVLAN_MODE_PASSTHRU:
> > + case MACVLAN_MODE_PASSTHRU_NOPROMISC:
> > break;
> > default:
> > return -EINVAL;
> > @@ -711,7 +758,8 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
> > if (data && data[IFLA_MACVLAN_MODE])
> > vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> >
> > - if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> > + if ((vlan->mode == MACVLAN_MODE_PASSTHRU) ||
> > + (vlan->mode == MACVLAN_MODE_PASSTHRU_NOPROMISC)) {
> > if (port->count)
> > return -EINVAL;
> > port->passthru = true;
> > diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> > index 2f4fa93..db67b9d 100644
> > --- a/include/linux/if_link.h
> > +++ b/include/linux/if_link.h
> > @@ -265,6 +265,7 @@ enum macvlan_mode {
> > MACVLAN_MODE_VEPA = 2, /* talk to other ports through ext bridge */
> > MACVLAN_MODE_BRIDGE = 4, /* talk to bridge ports directly */
> > MACVLAN_MODE_PASSTHRU = 8,/* take over the underlying device */
> > + MACVLAN_MODE_PASSTHRU_NOPROMISC = 16, /* passthru without promisc */
> > };
> >
> > /* SR-IOV virtual function management section */
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Eric Dumazet @ 2012-04-10 8:27 UTC (permalink / raw)
To: monstr; +Cc: netdev, John Williams, David Miller
In-Reply-To: <4F83EB0E.4020104@monstr.eu>
On Tue, 2012-04-10 at 10:10 +0200, Michal Simek wrote:
> Hi Eric,
>
> I have spent some time to investigate what it is causing regression on our network bechmark
> and I have identified that your patch "net: more accurate skb truesize" is causing that.
>
> I see regression especially on incoming tcp (RX) connection which is 20%. On TX I see regression till 5%.
>
> In general microblaze systems are pretty sensitive for memory usage and working with it.
> Increasing packet sizes has big impact on performance.
> I was surprised that regression is so high.
>
> From microblaze point of view is more important to remove that performance regression
> than having more accurate memory accounting. 20% performance regression is simple so high
> for that.
> Isn't there any other way to doing better memory accounting?
> Or is there any workaround which would be possible to use?
>
> Thanks,
> Michal
>
Hi Michal
We are currently working on the issue.
TCP stack was a bit optimistic in the ideal skb->len / skb->truesize
ratio that happened to 'mostly work' before my patch, but not anymore
after it.
memory accounting was wrong, we really wanted to be accurate, or risk
OOM and crashes.
Now we must fix tcp.
In the meantime you could try :
echo 1 >/proc/sys/net/ipv4/tcp_adv_win_scale
BTW, some NIC drivers are known to provide fat skb in their rx path, and
need to be fixed as well. (Some others just lie about skb->truesize to
avoid the tcp slowdown, see my previous iwlwifi patch)
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Eric Dumazet @ 2012-04-10 8:32 UTC (permalink / raw)
To: monstr; +Cc: netdev, John Williams, David Miller
In-Reply-To: <1334046444.3126.12.camel@edumazet-glaptop>
On Tue, 2012-04-10 at 10:27 +0200, Eric Dumazet wrote:
> BTW, some NIC drivers are known to provide fat skb in their rx path, and
> need to be fixed as well. (Some others just lie about skb->truesize to
> avoid the tcp slowdown, see my previous iwlwifi patch)
>
>
What is the driver you currently use on your platform ?
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Michal Simek @ 2012-04-10 8:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, John Williams, David Miller
In-Reply-To: <1334046444.3126.12.camel@edumazet-glaptop>
On 04/10/2012 10:27 AM, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 10:10 +0200, Michal Simek wrote:
>> Hi Eric,
>>
>> I have spent some time to investigate what it is causing regression on our network bechmark
>> and I have identified that your patch "net: more accurate skb truesize" is causing that.
>>
>> I see regression especially on incoming tcp (RX) connection which is 20%. On TX I see regression till 5%.
>>
>> In general microblaze systems are pretty sensitive for memory usage and working with it.
>> Increasing packet sizes has big impact on performance.
>> I was surprised that regression is so high.
>>
>> From microblaze point of view is more important to remove that performance regression
>> than having more accurate memory accounting. 20% performance regression is simple so high
>> for that.
>> Isn't there any other way to doing better memory accounting?
>> Or is there any workaround which would be possible to use?
>>
>> Thanks,
>> Michal
>>
>
> Hi Michal
>
> We are currently working on the issue.
>
> TCP stack was a bit optimistic in the ideal skb->len / skb->truesize
> ratio that happened to 'mostly work' before my patch, but not anymore
> after it.
>
> memory accounting was wrong, we really wanted to be accurate, or risk
> OOM and crashes.
>
> Now we must fix tcp.
>
> In the meantime you could try :
>
> echo 1>/proc/sys/net/ipv4/tcp_adv_win_scale
will retest it and let you know.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Michal Simek @ 2012-04-10 8:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, John Williams, David Miller
In-Reply-To: <1334046746.3126.13.camel@edumazet-glaptop>
On 04/10/2012 10:32 AM, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 10:27 +0200, Eric Dumazet wrote:
>
>> BTW, some NIC drivers are known to provide fat skb in their rx path, and
>> need to be fixed as well. (Some others just lie about skb->truesize to
>> avoid the tcp slowdown, see my previous iwlwifi patch)
>>
>>
>
> What is the driver you currently use on your platform ?
Using Xilinx ll_temac(in mainline) and axi_emac.
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: a F-RTO question
From: Ilpo Järvinen @ 2012-04-10 8:34 UTC (permalink / raw)
To: Li Yu; +Cc: Chao Pei, Netdev
In-Reply-To: <4F728EC9.1050302@gmail.com>
On Wed, 28 Mar 2012, Li Yu wrote:
> Even, the reason is latter, it also means the netowrk already is
> recovered from temporarily congestion or disordered state, so we also should
> not enter loss state.
There are also other considerations in this btw, some devices are simply
broken and go to RTO loop increasing the RTOs exponentially if we wouldn't
force retransmit of the next segment (one of those newly sent segments)
that isn't strictly mandatory imho. ...We used to do something more
clever here and avoided that retransmission but learned the hard way that
there's simply no way around it.
--
i.
^ permalink raw reply
* re: Ethernet driver for the WIZnet W5100 chip
From: Dan Carpenter @ 2012-04-10 8:40 UTC (permalink / raw)
To: msink; +Cc: netdev
Hello Mike Sinkovsky,
This is a semi-automatic email about new static checker warnings.
The patch 8b1467a31343: "Ethernet driver for the WIZnet W5100 chip"
from Apr 4, 2012, leads to the following Smatch complaint:
drivers/net/ethernet/wiznet/w5100.c:685 w5100_hw_probe()
error: we previously assumed 'data' could be null (see line 637)
drivers/net/ethernet/wiznet/w5100.c
636
637 if (data && is_valid_ether_addr(data->mac_addr)) {
^^^^
New check.
638 memcpy(ndev->dev_addr, data->mac_addr, ETH_ALEN);
639 } else {
640 random_ether_addr(ndev->dev_addr);
641 ndev->addr_assign_type |= NET_ADDR_RANDOM;
642 }
643
644 mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
645 if (!mem)
646 return -ENXIO;
647 mem_size = resource_size(mem);
648 if (!devm_request_mem_region(&pdev->dev, mem->start, mem_size, name))
649 return -EBUSY;
650 priv->base = devm_ioremap(&pdev->dev, mem->start, mem_size);
651 if (!priv->base)
652 return -EBUSY;
653
654 spin_lock_init(&priv->reg_lock);
655 priv->indirect = mem_size < W5100_BUS_DIRECT_SIZE;
656 if (priv->indirect) {
657 priv->read = w5100_read_indirect;
658 priv->write = w5100_write_indirect;
659 priv->read16 = w5100_read16_indirect;
660 priv->write16 = w5100_write16_indirect;
661 priv->readbuf = w5100_readbuf_indirect;
662 priv->writebuf = w5100_writebuf_indirect;
663 } else {
664 priv->read = w5100_read_direct;
665 priv->write = w5100_write_direct;
666 priv->read16 = w5100_read16_direct;
667 priv->write16 = w5100_write16_direct;
668 priv->readbuf = w5100_readbuf_direct;
669 priv->writebuf = w5100_writebuf_direct;
670 }
671
672 w5100_hw_reset(priv);
673 if (w5100_read16(priv, W5100_RTR) != RTR_DEFAULT)
674 return -ENODEV;
675
676 irq = platform_get_irq(pdev, 0);
677 if (irq < 0)
678 return irq;
679 ret = request_irq(irq, w5100_interrupt,
680 IRQ_TYPE_LEVEL_LOW, name, ndev);
681 if (ret < 0)
682 return ret;
683 priv->irq = irq;
684
685 priv->link_gpio = data->link_gpio;
^^^^^^^^^^^^^^^
New unchecked dereference.
686 if (gpio_is_valid(priv->link_gpio)) {
687 char *link_name = devm_kzalloc(&pdev->dev, 16, GFP_KERNEL);
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-10 8:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Michał Mirosław, Ben Hutchings,
Herbert Xu
In-Reply-To: <1334044558.3126.5.camel@edumazet-glaptop>
On Tue, Apr 10, 2012 at 09:55:58AM +0200, Eric Dumazet wrote:
> On Sun, 2012-04-08 at 20:13 +0300, Michael S. Tsirkin wrote:
> > commit 0110d6f22f392f976e84ab49da1b42f85b64a3c5
> > tun: orphan an skb on tx
> > Fixed a configuration where skbs get queued
> > at the tun device forever, blocking senders.
> >
> > However this fix isn't waterproof:
> > userspace can control whether the interface
> > is stopped, and if it is, packets
> > get queued in the qdisc, again potentially forever.
> >
> > Complete the fix by setting a private flag and orphaning
> > at the qdisc level.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/net/tun.c | 3 +++
> > include/linux/if.h | 1 +
> > net/core/dev.c | 5 +++++
> > net/sched/sch_generic.c | 5 +++++
> > 4 files changed, 14 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index bb8c72c..15c5bb8 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -535,6 +535,9 @@ static void tun_net_init(struct net_device *dev)
> > dev->tx_queue_len = TUN_READQ_SIZE; /* We prefer our own queue length */
> > break;
> > }
> > + /* Once queue becomes full, we stop tx until userspace
> > + * dequeues some packets, that is potentially forever. */
> > + dev->priv_flags |= IFF_TX_CAN_STALL;
> > }
> >
> > /* Character device part */
> > diff --git a/include/linux/if.h b/include/linux/if.h
> > index f995c66..dd2c7f7 100644
> > --- a/include/linux/if.h
> > +++ b/include/linux/if.h
> > @@ -81,6 +81,7 @@
> > #define IFF_UNICAST_FLT 0x20000 /* Supports unicast filtering */
> > #define IFF_TEAM_PORT 0x40000 /* device used as team port */
> > #define IFF_SUPP_NOFCS 0x80000 /* device supports sending custom FCS */
> > +#define IFF_TX_CAN_STALL 0x100000 /* Device can stop tx forever */
> >
> >
> > #define IF_GET_IFACE 0x0001 /* for querying only */
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 5d59155..e812706 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -2516,6 +2516,11 @@ int dev_queue_xmit(struct sk_buff *skb)
> > struct Qdisc *q;
> > int rc = -ENOMEM;
> >
> > + /* Orphan the skb - required if we might hang on to it
> > + * for indefinite time. */
> > + if (dev->priv_flags & IFF_TX_CAN_STALL)
> > + skb_orphan(skb);
> > +
> > /* Disable soft irqs for various locks below. Also
> > * stops preemption for RCU.
> > */
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 67fc573..27883d1 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -120,6 +120,11 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
> > /* And release qdisc */
> > spin_unlock(root_lock);
> >
> > + /* Orphan the skb - required if we might hang on to it
> > + * for indefinite time. */
> > + if (dev->priv_flags & IFF_TX_CAN_STALL)
> > + skb_orphan(skb);
> > +
> > HARD_TX_LOCK(dev, txq, smp_processor_id());
> > if (!netif_xmit_frozen_or_stopped(txq))
> > ret = dev_hard_start_xmit(skb, dev, txq);
>
> This slows down the core fastpath for a very specific use.
You are right.
I presume the fastpath is when the device is *not* stalled,
correct? So maybe it's ok to add this logic on slow path
where the queue is stopped and we queue the packet?
When the queue is running tun can orphan the skb itself.
This is what the change at the bottom of this mail on top of my
patch does - untested yet and naturally needs to be combined
and resubmitted properly, assuming it makes sense.
Would this, in your opinion, address this concern adequately?
Thanks!
> In your case I would just not use qdisc at all, like other virtual
> devices.
I think that if we do this, this also disables gso
for the device, doesn't it?
If true that would be a problem as this would
hurt performance of virtualized setups a lot.
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index bb8c72c..fd8c7f0 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -396,7 +396,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> sk_filter(tun->socket.sk, skb))
> goto drop;
>
> - if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= dev->tx_queue_len) {
> + if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= TUN_READQ_SIZE) {
> if (!(tun->flags & TUN_ONE_QUEUE)) {
> /* Normal queueing mode. */
> /* Packet scheduler handles dropping of further packets. */
tx_queue_len is controllable by SIOCSIFTXQLEN
so we'll need to override SIOCSIFTXQLEN somehow
to avoid breaking userspace that actually uses SIOCSIFTXQLEN, right?
> @@ -521,7 +521,7 @@ static void tun_net_init(struct net_device *dev)
> /* Zero header length */
> dev->type = ARPHRD_NONE;
> dev->flags = IFF_POINTOPOINT | IFF_NOARP | IFF_MULTICAST;
> - dev->tx_queue_len = TUN_READQ_SIZE; /* We prefer our own queue length */
> + dev->tx_queue_len = 0;
> break;
>
> case TUN_TAP_DEV:
> @@ -532,7 +532,7 @@ static void tun_net_init(struct net_device *dev)
>
> eth_hw_addr_random(dev);
>
> - dev->tx_queue_len = TUN_READQ_SIZE; /* We prefer our own queue length */
> + dev->tx_queue_len = 0;
> break;
> }
> }
>
I presume the fastpath is when the device is *not* stalled,
correct? So maybe the following on top of my patch - untested
and naturally would need to be combined and resubmitted properly -
would address the concern?
Thanks for the review!
diff --git a/net/core/dev.c b/net/core/dev.c
index 9d713b8..e42529b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2455,6 +2455,11 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
rc = NET_XMIT_SUCCESS;
} else {
skb_dst_force(skb);
+ /* Orphan the skb - required if we might hang on to it
+ * for indefinite time. */
+ if (unlikely(dev->priv_flags & IFF_TX_CAN_STALL))
+ skb_orphan(skb);
+
rc = q->enqueue(skb, q) & NET_XMIT_MASK;
if (qdisc_run_begin(q)) {
if (unlikely(contended)) {
@@ -2517,11 +2522,6 @@ int dev_queue_xmit(struct sk_buff *skb)
struct Qdisc *q;
int rc = -ENOMEM;
- /* Orphan the skb - required if we might hang on to it
- * for indefinite time. */
- if (dev->priv_flags & IFF_TX_CAN_STALL)
- skb_orphan(skb);
-
/* Disable soft irqs for various locks below. Also
* stops preemption for RCU.
*/
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 27883d1..ccc6137 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -120,14 +120,11 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
/* And release qdisc */
spin_unlock(root_lock);
- /* Orphan the skb - required if we might hang on to it
- * for indefinite time. */
- if (dev->priv_flags & IFF_TX_CAN_STALL)
- skb_orphan(skb);
-
HARD_TX_LOCK(dev, txq, smp_processor_id());
- if (!netif_xmit_frozen_or_stopped(txq))
+ if (likely(!netif_xmit_frozen_or_stopped(txq)))
ret = dev_hard_start_xmit(skb, dev, txq);
+ else if (dev->priv_flags & IFF_TX_CAN_STALL)
+ skb_orphan(skb);
HARD_TX_UNLOCK(dev, txq);
^ permalink raw reply related
* Re: net: more accurate skb truesize - regression on Microblaze
From: Eric Dumazet @ 2012-04-10 8:45 UTC (permalink / raw)
To: monstr; +Cc: netdev, John Williams, David Miller
In-Reply-To: <4F83F166.4010208@monstr.eu>
On Tue, 2012-04-10 at 10:37 +0200, Michal Simek wrote:
> On 04/10/2012 10:32 AM, Eric Dumazet wrote:
> > On Tue, 2012-04-10 at 10:27 +0200, Eric Dumazet wrote:
> >
> >> BTW, some NIC drivers are known to provide fat skb in their rx path, and
> >> need to be fixed as well. (Some others just lie about skb->truesize to
> >> avoid the tcp slowdown, see my previous iwlwifi patch)
> >>
> >>
> >
> > What is the driver you currently use on your platform ?
>
> Using Xilinx ll_temac(in mainline) and axi_emac.
>
OK thanks, could you please give us the content
of /proc/sys/net/ipv4/tcp_rmem ?
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Eric Dumazet @ 2012-04-10 8:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Michał Mirosław, Ben Hutchings,
Herbert Xu
In-Reply-To: <20120410084151.GA27193@redhat.com>
On Tue, 2012-04-10 at 11:41 +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 09:55:58AM +0200, Eric Dumazet wrote:
> > In your case I would just not use qdisc at all, like other virtual
> > devices.
>
> I think that if we do this, this also disables gso
> for the device, doesn't it?
Not at all, thats unrelated.
> If true that would be a problem as this would
> hurt performance of virtualized setups a lot.
In fact, removing qdisc layer will help a lot, removing a contention
point.
Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
be always empty. Whats the point storing more than 500 packets for a
device ? Thats a latency killer.
>
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index bb8c72c..fd8c7f0 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -396,7 +396,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> > sk_filter(tun->socket.sk, skb))
> > goto drop;
> >
> > - if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= dev->tx_queue_len) {
> > + if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= TUN_READQ_SIZE) {
> > if (!(tun->flags & TUN_ONE_QUEUE)) {
> > /* Normal queueing mode. */
> > /* Packet scheduler handles dropping of further packets. */
>
> tx_queue_len is controllable by SIOCSIFTXQLEN
> so we'll need to override SIOCSIFTXQLEN somehow
> to avoid breaking userspace that actually uses SIOCSIFTXQLEN, right?
Right now, you control with this tx_queue_len both the qdisc limit (if
pfifo_fast default) and the receive_queue in TUN.
That doesnt seem right to me, and more a hack/side effect.
Maybe you want to introduce a new setting, only controling receive queue
limit, and use tx_queue_len for its original meaning.
Then, setting tx_queue_len to 0 permits to remove qdisc layer, as any
other netdevice.
^ permalink raw reply
* Re: Ethernet driver for the WIZnet W5100 chip
From: Dan Carpenter @ 2012-04-10 9:08 UTC (permalink / raw)
To: msink; +Cc: netdev
The same issue is there in w5300_hw_probe() as well.
regards,
dan carpenter
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Michal Simek @ 2012-04-10 9:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, John Williams, David Miller
In-Reply-To: <1334047544.3126.14.camel@edumazet-glaptop>
On 04/10/2012 10:45 AM, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 10:37 +0200, Michal Simek wrote:
>> On 04/10/2012 10:32 AM, Eric Dumazet wrote:
>>> On Tue, 2012-04-10 at 10:27 +0200, Eric Dumazet wrote:
>>>
>>>> BTW, some NIC drivers are known to provide fat skb in their rx path, and
>>>> need to be fixed as well. (Some others just lie about skb->truesize to
>>>> avoid the tcp slowdown, see my previous iwlwifi patch)
>>>>
>>>>
>>>
>>> What is the driver you currently use on your platform ?
>>
>> Using Xilinx ll_temac(in mainline) and axi_emac.
>>
>
> OK thanks, could you please give us the content
> of /proc/sys/net/ipv4/tcp_rmem ?
~ # cat /proc/sys/net/ipv4/tcp_rmem
4096 87380 130048
Let me know if you want to test anything.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Eric Dumazet @ 2012-04-10 9:24 UTC (permalink / raw)
To: monstr; +Cc: netdev, John Williams, David Miller
In-Reply-To: <4F83F959.3070302@monstr.eu>
On Tue, 2012-04-10 at 11:11 +0200, Michal Simek wrote:
> ~ # cat /proc/sys/net/ipv4/tcp_rmem
> 4096 87380 130048
Are they default values, or tuned by admin ?
130048 bytes isnt enough to let TCP open its rcv window.
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Michal Simek @ 2012-04-10 9:29 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, John Williams, David Miller
In-Reply-To: <1334049896.3126.23.camel@edumazet-glaptop>
On 04/10/2012 11:24 AM, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 11:11 +0200, Michal Simek wrote:
>
>> ~ # cat /proc/sys/net/ipv4/tcp_rmem
>> 4096 87380 130048
>
> Are they default values, or tuned by admin ?
>
> 130048 bytes isnt enough to let TCP open its rcv window.
yep. Default value after powerup. What's wrong with that?
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-10 9:31 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Michał Mirosław, Ben Hutchings,
Herbert Xu
In-Reply-To: <1334048100.3126.21.camel@edumazet-glaptop>
On Tue, Apr 10, 2012 at 10:55:00AM +0200, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 11:41 +0300, Michael S. Tsirkin wrote:
> > On Tue, Apr 10, 2012 at 09:55:58AM +0200, Eric Dumazet wrote:
>
> > > In your case I would just not use qdisc at all, like other virtual
> > > devices.
> >
> > I think that if we do this, this also disables gso
> > for the device, doesn't it?
>
> Not at all, thats unrelated.
>
> > If true that would be a problem as this would
> > hurt performance of virtualized setups a lot.
>
> In fact, removing qdisc layer will help a lot, removing a contention
> point.
>
> Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> be always empty. Whats the point storing more than 500 packets for a
> device ? Thats a latency killer.
AKA bufferbloat :)
We could try and reduce the TUN queue so that qdisc can drop packets in
an intelligent manner (e.g. choke) but this would conflict with what you
propose, right?
> >
> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > index bb8c72c..fd8c7f0 100644
> > > --- a/drivers/net/tun.c
> > > +++ b/drivers/net/tun.c
> > > @@ -396,7 +396,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> > > sk_filter(tun->socket.sk, skb))
> > > goto drop;
> > >
> > > - if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= dev->tx_queue_len) {
> > > + if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= TUN_READQ_SIZE) {
> > > if (!(tun->flags & TUN_ONE_QUEUE)) {
> > > /* Normal queueing mode. */
> > > /* Packet scheduler handles dropping of further packets. */
> >
> > tx_queue_len is controllable by SIOCSIFTXQLEN
> > so we'll need to override SIOCSIFTXQLEN somehow
> > to avoid breaking userspace that actually uses SIOCSIFTXQLEN, right?
>
> Right now, you control with this tx_queue_len both the qdisc limit (if
> pfifo_fast default) and the receive_queue in TUN.
>
> That doesnt seem right to me, and more a hack/side effect.
> Maybe you want to introduce a new setting, only controling receive queue
> limit, and use tx_queue_len for its original meaning.
>
> Then, setting tx_queue_len to 0 permits to remove qdisc layer, as any
> other netdevice.
>
>
True. Still this is the only interface we have for controlling
the internal queue length so it seems safe to assume someone
is using it for this purpose.
And if that happens we get the deadlock back since
tx_queue_len will get set to a non-0 value. Right?
--
MST
^ 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