Netdev List
 help / color / mirror / Atom feed
* Re: send(), sendmsg(), sendto() not thread-safe
From: Stephen Hemminger @ 2006-05-15 23:35 UTC (permalink / raw)
  To: Rick Jones; +Cc: mark1smi, David S. Miller, netdev
In-Reply-To: <44690C1C.8030101@hp.com>

On Mon, 15 May 2006 16:17:48 -0700
Rick Jones <rick.jones2@hp.com> wrote:

> David S. Miller wrote:
> > From: Mark A Smith <mark1smi@us.ibm.com>
> > Date: Mon, 15 May 2006 14:39:06 -0700
> > 
> > 
> >>I discovered that in some cases, send(), sendmsg(), and sendto() are not
> >>thread-safe. Although the man page for these functions does not specify
> >>whether these functions are supposed to be thread-safe, my reading of the
> >>POSIX/SUSv3 specification tells me that they should be. I traced the
> >>problem to tcp_sendmsg(). I was very curious about this issue, so I wrote
> >>up a small page to describe in more detail my findings. You can find it at:
> >>http://www.almaden.ibm.com/cs/people/marksmith/sendmsg.html .
> 
> 
> # ./sendmsgclient localhost
> ERROR! We should have all 0! We don't!
> buff[16384]=1
> buff[16385]=1
> buff[16386]=1
> buff[16387]=1
> buff[16388]=1
> buff[16389]=1
> buff[16390]=1
> buff[16391]=1
> buff[16392]=1
> buff[16393]=1
> That's 10/32768 bad bytes
> # uname -a
> HP-UX tarry B.11.23 U ia64 2397028692 unlimited-user license
> 
> Given that the URL above asserts that HP-UX claims atomicity, either 
> there is a bug in the UX stack, or perhaps the test?  I took a quick 
> look at the HP-UX 11iv2 (aka 11.23) manpage for sendmsg and didn't see 
> anything about atomicity there - on which manpage(s) or docs was the 
> assertion of HP-UX atomicity made?
> 
> I presume this is only for "blocking" sockets?  I cannot at least off 
> the top of my head see how a stack could offer it on non-blocking sockets.

The test seems to be based on sending a big message. In this case,
on non-blocking sockets, the send call will return partial status. The
return from the system call will be less than the number of bytes requested.

> 
> > And frankly, BSD defines BSD socket semantics here not some wording in
> > the POSIX standards.
> 
> Have BSD socket semantics ever been updated/clarified any any 
> quasi-official manner since the popular presence of threads?  Or 
> are/were Posix/Xopen filling a gap?
> 
> rick jones
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH 2/2] skge: don't allow transmit ring to be too small
From: Stephen Hemminger @ 2006-05-15 23:32 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: netdev
In-Reply-To: <20060515163025.1b81ecbb@localhost.localdomain>

The driver will get stuck (permanent transmit timeout), if the transmit
ring size is set too small.  It needs to have enough ring elements to
hold one maximum size transmit.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


--- skge-2.6.orig/drivers/net/skge.c
+++ skge-2.6/drivers/net/skge.c
@@ -402,7 +402,7 @@ static int skge_set_ring_param(struct ne
 	int err;
 
 	if (p->rx_pending == 0 || p->rx_pending > MAX_RX_RING_SIZE ||
-	    p->tx_pending == 0 || p->tx_pending > MAX_TX_RING_SIZE)
+	    p->tx_pending < MAX_SKB_FRAGS+1 || p->tx_pending > MAX_TX_RING_SIZE)
 		return -EINVAL;
 
 	skge->rx_ring.count = p->rx_pending;

^ permalink raw reply

* [PATCH 1/2] skge: bad checksums on big-endian platforms
From: Stephen Hemminger @ 2006-05-15 23:30 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: netdev

Skge driver always causes  bad checksums on big-endian.
The checksum in the receive control block was being swapped
when it doesn't need to be.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

--- skge-2.6.orig/drivers/net/skge.c
+++ skge-2.6/drivers/net/skge.c
@@ -2717,8 +2717,7 @@ static int skge_poll(struct net_device *
 		if (control & BMU_OWN)
 			break;
 
-		skb = skge_rx_get(skge, e, control, rd->status,
-				  le16_to_cpu(rd->csum2));
+		skb = skge_rx_get(skge, e, control, rd->status, rd->csum2);
 		if (likely(skb)) {
 			dev->last_rx = jiffies;
 			netif_receive_skb(skb);

^ permalink raw reply

* [PATCH] net_sched: potential jiffy wrap bug in dev_watchdog
From: Stephen Hemminger @ 2006-05-15 23:28 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev

There is a potential jiffy wraparound bug in the transmit watchdog
that is easily avoided by using time_after().

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


--- linux-2.6.orig/net/sched/sch_generic.c
+++ linux-2.6/net/sched/sch_generic.c
@@ -193,8 +193,10 @@ static void dev_watchdog(unsigned long a
 		    netif_running(dev) &&
 		    netif_carrier_ok(dev)) {
 			if (netif_queue_stopped(dev) &&
-			    (jiffies - dev->trans_start) > dev->watchdog_timeo) {
-				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n", dev->name);
+			    time_after(jiffies, dev->trans_start + dev->watchdog_timeo)) {
+
+				printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n",
+				       dev->name);
 				dev->tx_timeout(dev);
 			}
 			if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))

^ permalink raw reply

* Re: send(), sendmsg(), sendto() not thread-safe
From: Rick Jones @ 2006-05-15 23:17 UTC (permalink / raw)
  To: mark1smi; +Cc: David S. Miller, netdev
In-Reply-To: <20060515.154939.28171388.davem@davemloft.net>

David S. Miller wrote:
> From: Mark A Smith <mark1smi@us.ibm.com>
> Date: Mon, 15 May 2006 14:39:06 -0700
> 
> 
>>I discovered that in some cases, send(), sendmsg(), and sendto() are not
>>thread-safe. Although the man page for these functions does not specify
>>whether these functions are supposed to be thread-safe, my reading of the
>>POSIX/SUSv3 specification tells me that they should be. I traced the
>>problem to tcp_sendmsg(). I was very curious about this issue, so I wrote
>>up a small page to describe in more detail my findings. You can find it at:
>>http://www.almaden.ibm.com/cs/people/marksmith/sendmsg.html .


# ./sendmsgclient localhost
ERROR! We should have all 0! We don't!
buff[16384]=1
buff[16385]=1
buff[16386]=1
buff[16387]=1
buff[16388]=1
buff[16389]=1
buff[16390]=1
buff[16391]=1
buff[16392]=1
buff[16393]=1
That's 10/32768 bad bytes
# uname -a
HP-UX tarry B.11.23 U ia64 2397028692 unlimited-user license

Given that the URL above asserts that HP-UX claims atomicity, either 
there is a bug in the UX stack, or perhaps the test?  I took a quick 
look at the HP-UX 11iv2 (aka 11.23) manpage for sendmsg and didn't see 
anything about atomicity there - on which manpage(s) or docs was the 
assertion of HP-UX atomicity made?

I presume this is only for "blocking" sockets?  I cannot at least off 
the top of my head see how a stack could offer it on non-blocking sockets.

> And frankly, BSD defines BSD socket semantics here not some wording in
> the POSIX standards.

Have BSD socket semantics ever been updated/clarified any any 
quasi-official manner since the popular presence of threads?  Or 
are/were Posix/Xopen filling a gap?

rick jones

^ permalink raw reply

* Re: [PATCH] tcpdump may trace some outbound packets twice.
From: Ranjit Manomohan @ 2006-05-15 23:11 UTC (permalink / raw)
  To: David S. Miller; +Cc: ranjitm, akpm, linux-kernel, netdev
In-Reply-To: <20060515.142645.94689626.davem@davemloft.net>

On Mon, 15 May 2006, David S. Miller wrote:

> From: Ranjit Manomohan <ranjitm@google.com>
> Date: Mon, 15 May 2006 14:19:06 -0700 (PDT)
> 
> > Heres a new version which does a copy instead of the clone to avoid
> > the double cloning issue.
> 
> I still very much dislike this patch because it is creating
> 1 more clone per packet than is actually necessary and that
> is very expensive.
> 
> dev_queue_xmit_nit() is going to clone whatever SKB you send into
> there, so better to just bump the reference count (with skb_get())
> instead of cloning or copying.
> 

I was a bit apprehensive about just incrementing the refcnt but that works 
too. Attached is the modified version.

-Thanks,
Ranjit

--- linux-2.6/net/sched/sch_generic.c	2006-05-10 12:34:52.000000000 -0700
+++ linux/net/sched/sch_generic.c	2006-05-15 15:48:03.000000000 -0700
@@ -136,8 +136,12 @@
 
 			if (!netif_queue_stopped(dev)) {
 				int ret;
+				struct sk_buff *skbc = NULL;
+				/* Increment the reference count on the skb so
+				 * that we can use it after a successful xmit.
+				 */
 				if (netdev_nit)
-					dev_queue_xmit_nit(skb, dev);
+					skbc = skb_get(skb);
 
 				ret = dev->hard_start_xmit(skb, dev);
 				if (ret == NETDEV_TX_OK) { 
@@ -145,9 +149,20 @@
 						dev->xmit_lock_owner = -1;
 						spin_unlock(&dev->xmit_lock);
 					}
+					if (skbc) {
+						/* transmit succeeded, 
+						 * trace the buffer. */
+						dev_queue_xmit_nit(skbc,dev);
+						kfree_skb(skbc);
+					}
 					spin_lock(&dev->queue_lock);
 					return -1;
 				}
+
+				/* Call free in case we incremented refcnt */
+				if (skbc)
+					kfree_skb(skbc);
+
 				if (ret == NETDEV_TX_LOCKED && nolock) {
 					spin_lock(&dev->queue_lock);
 					goto collision; 

^ permalink raw reply

* Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
From: Rick Jones @ 2006-05-15 23:01 UTC (permalink / raw)
  To: David S. Miller; +Cc: rdunlap, borntrae, netdev
In-Reply-To: <20060515.154955.114682456.davem@davemloft.net>

David S. Miller wrote:
> From: Rick Jones <rick.jones2@hp.com>
> Date: Mon, 15 May 2006 14:39:23 -0700
> 
> 
>>How about:
> 
> 
> How about, just leave it alone? :-)

That would work too :) but I guess I figured based on the reason given 
just before I posted, for why setting to L1_CACHE_SIZE was wrong that 
there was an implication that tweaking it might be right.

rick

^ permalink raw reply

* [PATCH] e1000: endian fixes
From: Alexey Dobriyan @ 2006-05-15 22:54 UTC (permalink / raw)
  To: Andrew Morton; +Cc: cramerj, netdev, Jeff Garzik

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 drivers/net/e1000/e1000_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- 1/drivers/net/e1000/e1000_main.c
+++ 1/drivers/net/e1000/e1000_main.c
@@ -2451,7 +2451,7 @@ e1000_tso(struct e1000_adapter *adapter,
 
 		hdr_len = ((skb->h.raw - skb->data) + (skb->h.th->doff << 2));
 		mss = skb_shinfo(skb)->tso_size;
-		if (skb->protocol == ntohs(ETH_P_IP)) {
+		if (skb->protocol == htons(ETH_P_IP)) {
 			skb->nh.iph->tot_len = 0;
 			skb->nh.iph->check = 0;
 			skb->h.th->check =
@@ -2930,7 +2930,7 @@ e1000_xmit_frame(struct sk_buff *skb, st
 	/* Old method was to assume IPv4 packet by default if TSO was enabled.
 	 * 82571 hardware supports TSO capabilities for IPv6 as well...
 	 * no longer assume, we must. */
-	if (likely(skb->protocol == ntohs(ETH_P_IP)))
+	if (likely(skb->protocol == htons(ETH_P_IP)))
 		tx_flags |= E1000_TX_FLAGS_IPV4;
 
 	e1000_tx_queue(adapter, tx_ring, tx_flags,


^ permalink raw reply

* Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
From: David S. Miller @ 2006-05-15 22:49 UTC (permalink / raw)
  To: rick.jones2; +Cc: rdunlap, borntrae, netdev
In-Reply-To: <4468F50B.9000102@hp.com>

From: Rick Jones <rick.jones2@hp.com>
Date: Mon, 15 May 2006 14:39:23 -0700

> How about:

How about, just leave it alone? :-)

^ permalink raw reply

* Re: send(), sendmsg(), sendto() not thread-safe
From: David S. Miller @ 2006-05-15 22:49 UTC (permalink / raw)
  To: mark1smi; +Cc: linux-kernel, netdev
In-Reply-To: <OFE8460E54.0C8D85D8-ON8525716F.0074F22F-8825716F.0076D537@us.ibm.com>

From: Mark A Smith <mark1smi@us.ibm.com>
Date: Mon, 15 May 2006 14:39:06 -0700

> I discovered that in some cases, send(), sendmsg(), and sendto() are not
> thread-safe. Although the man page for these functions does not specify
> whether these functions are supposed to be thread-safe, my reading of the
> POSIX/SUSv3 specification tells me that they should be. I traced the
> problem to tcp_sendmsg(). I was very curious about this issue, so I wrote
> up a small page to describe in more detail my findings. You can find it at:
> http://www.almaden.ibm.com/cs/people/marksmith/sendmsg.html .

I don't understand why the desire is so high to ensure that
individual threads get "atomic" writes, you can't even ensure
that in the general case.

Only sloppy programs that don't do their own internal locking hit into
issues in this area.

>From your findings, the vast majority of systems you investigated do
not provide "atomic" thread safe write semantics over TCP sockets.
And frankly, BSD defines BSD socket semantics here not some wording in
the POSIX standards.

Finally, this discussion belongs on the networking development mailing
list, netdev@vger.kernel.org, not linux-kernel.

^ permalink raw reply

* Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
From: Stephen Hemminger @ 2006-05-15 21:58 UTC (permalink / raw)
  To: Rick Jones; +Cc: David S. Miller, rdunlap, borntrae, netdev
In-Reply-To: <4468F50B.9000102@hp.com>

On Mon, 15 May 2006 14:39:23 -0700
Rick Jones <rick.jones2@hp.com> wrote:

> David S. Miller wrote:
> > From: "Randy.Dunlap" <rdunlap@xenotime.net>
> > Date: Mon, 15 May 2006 08:02:58 -0700
> > 
> > 
> >>>-#define	NETDEV_ALIGN		32
> >>>+#define	NETDEV_ALIGN		L1_CACHE_BYTES
> >>> #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
> >>
> >>I don't know about the fixed value of 32, but if this patch is
> >>accepted, I'd prefer NETDEV_ALIGN_MASK instead of NETDEV_ALIGN_CONST.
> > 
> > 
> > The reason it's 32 is that old drivers depended on the
> > struct being at least 32-byte aligned because they would
> > embed structures DMA'd to/from the card in their private
> > area and just assumed that would be aligned enough for
> > the card's restrictions.
> > 
> > So setting it to L1_CACHE_BYTES would be wrong, because if
> > that happens to be less than 32 it would violate said
> > assumption which we are catering to.
> 
> How about:
> 
> #define NETDEV_ALIGN_MIN 32
> #if L1_CACHE_BYTES > NETDEV_ALIGN_MIN
> # define NETDEV_ALIGN L1_CACHE_BYTES
> #else
> # define NETDEV_ALIGN NETDEV_ALIGN_MIN
> #endif
> 

I can't see how adding more padding could help performance here.
Most drivers reference both parts (netdevice and netdev_priv) in
most code paths. So having the stuff on separate cache lines couldn't
help that much. 

Alternatively, there might be a performance gain if the netdevice stats
structure was rearranged so the rx and tx stats were on different cache lines.

^ permalink raw reply

* Re: skge driver oops
From: David Arnold @ 2006-05-15 21:55 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Arnold, netdev
In-Reply-To: <20060512085339.72faaa44@localhost.localdomain>

-->"Stephen" == Stephen Hemminger <shemminger@osdl.org> writes:

  >> i've been getting semi-regular lockups on my machine over 2.6.16
  >> series.  I recently attached a serial console in an attempt to
  >> capture an OOPS.

  Stephen> Could you retest with the v1.5 version that is 2.6.17-rc3?

using 2.6.17-rc4, i got

  BUG: unable to handle kernel NULL pointer dereference at virtual address 090
   printing eip:
  c11982de
  *pde = 0
  Oops: 0000 [#1]
  SMP
  Modules linked in: hci_usb i2c_amd756
  CPU: 0
  EIP: 0060:[<c11982de>] Not tainted VLI
  EFLAGS: 10283 (2.6.17rc4da #19)
  EIP is at skge_tx_complete+0x1e/0x50
  eax: 00000046 ebx: f772eb48 ecx: 00000000 edx: 00000000
  esi: f772eb3c edi: f7dd0700 ebp: c1500f54 esp: c1500f44
  ds: 007b es: 007b ss: 0068
  Process swapper (pid: 0, threadinfo=c1500000 task=c1443d00)
  Stack: <0>f755d200 f7dd0700 f772eb3c f772eb3c c1500fa4 c119cecd 00000000 00000296
         c1500f98 c1034bcc f7baa204 79e0b937 f7de0f20 f7dd0724 f76c2864 00000040
         00000000 f7dd0714 c1500000 c8560600 c14f7de0 f7dd04c0 f7dd0400 c1d6bb94
  Call Trace:
    <c1004165> show_stack_log_lvl+0xa5/0xf0
    <c10043cb> show_registers+0x1ab/0x240
    <c10046ae> die+0x12e/0x2d0
    <c1014e2a> do_page_fault+0x20a/0x6dc
    <c1003a4f> error_code+0x4f/0x54
    <c119cecd> skge_poll+0xcd/0x5e0
    <c12608fb> net_rx_action+0x8b/0x120
    <c1023154> __do_softirq+0x74/0xe0
    <c1005adc> do_softirq+0x5c/0x60
    ========================
    <c1022e29> irq_exit+0x39/0x50
    <c1005a5c> do_IRQ+0x4c/0x70
    <c100391a> common_interrupt+0x1a/0x20
    <c1001c34> cpu_idle+0x64/0x80
    <c10002d5> rest_init+0x35/0c40
    <c14c07ef> start_kernel+0x2ef/0x420
    <c1000210> 0xc1000210
  Code: 66 b8 e8 03 c3 90 8d b4 26 00 00 00 00 55 89 e5 57 89 c7 56 89 d6 53 83 ec 04 8b 58 14 39 d3 74 31 8b 4b 08 c7
  EIP: [<c11982de>] skge_tx_complete+0x1e/0x50 SS:ESP 0068:c1500f44
    <0>Kernel panic - not syncing: Fatal exception in interrupt





d

^ permalink raw reply

* Re: SIOCSIWESSID + SIOCSIWAP behaviour
From: Jean Tourrilhes @ 2006-05-15 21:55 UTC (permalink / raw)
  To: Jouni Malinen; +Cc: Dan Williams, Daniel Drake, netdev, softmac-dev
In-Reply-To: <20060515214014.GA12019@instant802.com>

On Mon, May 15, 2006 at 02:40:14PM -0700, Jouni Malinen wrote:
> On Mon, May 15, 2006 at 01:28:13PM -0700, Jean Tourrilhes wrote:
> 
> > 	I believe the BSSID has to be unique. HP APs can also offer
> > multiple ESSID for the same BSSID, but they do so using different
> > BSSID. If you look at the 802.11 spec, I can't see how two different
> > virtual cells can have the same BSSID, because the BSSID is the only
> > thing that identify the cell (the ESSID is not in each packet).
> 
> What the standard says is somewhat irrelevant since there are APs that
> use multiple SSID with the same BSSID. I would say that it is reasonable
> for users to expect that this works with Linux, too.

	Yeah, I guess it's required when you are using standard client
card, but using the same BSSID is not as clean.

> > 	Personally, I think that SIWAP is too tricky to use properly
> > for most users, so we should encourage them to not use it.
> 
> Keep in mind that it is also used by some programs (e.g., wpa_supplicant
> in ap_scan=1) without the user having to know anything about that level
> of details..

	My point is that we don't have to be perfect with respect to
the end user through SIWAP.

> > 	There is still a performance issue. Having to cancel and
> > restart associations waste CPU cycles. One way to deal with that would
> > be to improve the "commit" strategy of the Wireless API. Basically,
> > every time you do a set, you schedule or reschedule the "commit" ioctl
> > to happen in a few dozen ms. This way, you could bundle all the
> > settting of apps such as wpa_supplicant with only a single
> > commit. This is similar to the way the routing API works.
> 
> Number of drivers use an atomic "associate" command (usually, a private
> ioctl) with wpa_supplicant. If there were a standard way of doing that,
> it could be helpful.

	My goal was to offer the "commit" mechanism to solve those
issues. The basic plumbing is in place, so we just a slightly more
clever "commit" mechanism. The advantage is that the mechanism is
generic, so will work for anything.

> Jouni Malinen                                            PGP id EFC895FA

	Jean

^ permalink raw reply

* Re: SIOCSIWESSID + SIOCSIWAP behaviour
From: Jouni Malinen @ 2006-05-15 21:40 UTC (permalink / raw)
  To: Jean Tourrilhes
  Cc: Dan Williams, Daniel Drake, Jouni Malinen, netdev, softmac-dev
In-Reply-To: <20060515202813.GA7400@bougret.hpl.hp.com>

On Mon, May 15, 2006 at 01:28:13PM -0700, Jean Tourrilhes wrote:

> 	I believe the BSSID has to be unique. HP APs can also offer
> multiple ESSID for the same BSSID, but they do so using different
> BSSID. If you look at the 802.11 spec, I can't see how two different
> virtual cells can have the same BSSID, because the BSSID is the only
> thing that identify the cell (the ESSID is not in each packet).

What the standard says is somewhat irrelevant since there are APs that
use multiple SSID with the same BSSID. I would say that it is reasonable
for users to expect that this works with Linux, too.

> 	Personally, I think that SIWAP is too tricky to use properly
> for most users, so we should encourage them to not use it.

Keep in mind that it is also used by some programs (e.g., wpa_supplicant
in ap_scan=1) without the user having to know anything about that level
of details..

> 	There is still a performance issue. Having to cancel and
> restart associations waste CPU cycles. One way to deal with that would
> be to improve the "commit" strategy of the Wireless API. Basically,
> every time you do a set, you schedule or reschedule the "commit" ioctl
> to happen in a few dozen ms. This way, you could bundle all the
> settting of apps such as wpa_supplicant with only a single
> commit. This is similar to the way the routing API works.

Number of drivers use an atomic "associate" command (usually, a private
ioctl) with wpa_supplicant. If there were a standard way of doing that,
it could be helpful.

-- 
Jouni Malinen                                            PGP id EFC895FA

^ permalink raw reply

* Re: [PATCH] tcpdump may trace some outbound packets twice.
From: Patrick McHardy @ 2006-05-15 21:41 UTC (permalink / raw)
  To: David S. Miller; +Cc: ranjitm, akpm, linux-kernel, netdev
In-Reply-To: <20060515.142645.94689626.davem@davemloft.net>

David S. Miller wrote:
> From: Ranjit Manomohan <ranjitm@google.com>
> Date: Mon, 15 May 2006 14:19:06 -0700 (PDT)
> 
> 
>>Heres a new version which does a copy instead of the clone to avoid
>>the double cloning issue.
> 
> 
> I still very much dislike this patch because it is creating
> 1 more clone per packet than is actually necessary and that
> is very expensive.
> 
> dev_queue_xmit_nit() is going to clone whatever SKB you send into
> there, so better to just bump the reference count (with skb_get())
> instead of cloning or copying.


I think this would break the tc actions. Some actions call
pskb_expand_head() on input, which BUGs on skb_shared(skb).
They can't clone the skb instead because the functions
doing that don't own it, the caller would continue with the
old skb.

^ permalink raw reply

* Re: Hardware button support for Wireless cards
From: Ivo van Doorn @ 2006-05-15 21:42 UTC (permalink / raw)
  To: Jason Lunz; +Cc: Dan Williams, Michael Buesch, netdev
In-Reply-To: <20060515194614.GB25683@knob.reflex>

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On Monday 15 May 2006 21:46, Jason Lunz wrote:
> On Mon, May 15, 2006 at 03:12:50PM -0400, Dan Williams wrote:
> > When those bits get set in the register, is the radio already disabled?
> > ie, for the bcm43xx chip, is it really force-disabled by the button, or
> > does the driver have some control?
> 
> I assume it disables the radio without any help from the driver, because
> it definitely turns off my radio, but the bcm43xx driver i'm using (in
> 2.6.17-rc4) currently doesn't pay any attention to those registers
> afaik.

Correct, my laptop with an integrated BCM43xx chip works this was as well.
I can use the button to enable and disable the device (led is switching on and off)
without a bcm43xx driver installed. Hardware detection just picks it up when
I enable the device with the button as new device. So the hardware button works
directly with the hardware.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
From: Rick Jones @ 2006-05-15 21:39 UTC (permalink / raw)
  To: David S. Miller; +Cc: rdunlap, borntrae, netdev
In-Reply-To: <20060515.143011.36635008.davem@davemloft.net>

David S. Miller wrote:
> From: "Randy.Dunlap" <rdunlap@xenotime.net>
> Date: Mon, 15 May 2006 08:02:58 -0700
> 
> 
>>>-#define	NETDEV_ALIGN		32
>>>+#define	NETDEV_ALIGN		L1_CACHE_BYTES
>>> #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
>>
>>I don't know about the fixed value of 32, but if this patch is
>>accepted, I'd prefer NETDEV_ALIGN_MASK instead of NETDEV_ALIGN_CONST.
> 
> 
> The reason it's 32 is that old drivers depended on the
> struct being at least 32-byte aligned because they would
> embed structures DMA'd to/from the card in their private
> area and just assumed that would be aligned enough for
> the card's restrictions.
> 
> So setting it to L1_CACHE_BYTES would be wrong, because if
> that happens to be less than 32 it would violate said
> assumption which we are catering to.

How about:

#define NETDEV_ALIGN_MIN 32
#if L1_CACHE_BYTES > NETDEV_ALIGN_MIN
# define NETDEV_ALIGN L1_CACHE_BYTES
#else
# define NETDEV_ALIGN NETDEV_ALIGN_MIN
#endif

rick jones

^ permalink raw reply

* Re: [RFC] changing value of NETDEV_ALIGN to cacheline size
From: David S. Miller @ 2006-05-15 21:30 UTC (permalink / raw)
  To: rdunlap; +Cc: borntrae, netdev
In-Reply-To: <20060515080258.bdcfba5f.rdunlap@xenotime.net>

From: "Randy.Dunlap" <rdunlap@xenotime.net>
Date: Mon, 15 May 2006 08:02:58 -0700

> > -#define	NETDEV_ALIGN		32
> > +#define	NETDEV_ALIGN		L1_CACHE_BYTES
> >  #define	NETDEV_ALIGN_CONST	(NETDEV_ALIGN - 1)
> 
> I don't know about the fixed value of 32, but if this patch is
> accepted, I'd prefer NETDEV_ALIGN_MASK instead of NETDEV_ALIGN_CONST.

The reason it's 32 is that old drivers depended on the
struct being at least 32-byte aligned because they would
embed structures DMA'd to/from the card in their private
area and just assumed that would be aligned enough for
the card's restrictions.

So setting it to L1_CACHE_BYTES would be wrong, because if
that happens to be less than 32 it would violate said
assumption which we are catering to.

^ permalink raw reply

* Re: [PATCH] tcpdump may trace some outbound packets twice.
From: David S. Miller @ 2006-05-15 21:26 UTC (permalink / raw)
  To: ranjitm; +Cc: akpm, linux-kernel, netdev
In-Reply-To: <Pine.LNX.4.56.0605151409110.25064@ranjit.corp.google.com>

From: Ranjit Manomohan <ranjitm@google.com>
Date: Mon, 15 May 2006 14:19:06 -0700 (PDT)

> Heres a new version which does a copy instead of the clone to avoid
> the double cloning issue.

I still very much dislike this patch because it is creating
1 more clone per packet than is actually necessary and that
is very expensive.

dev_queue_xmit_nit() is going to clone whatever SKB you send into
there, so better to just bump the reference count (with skb_get())
instead of cloning or copying.

^ permalink raw reply

* Re: [PATCH] tcpdump may trace some outbound packets twice.
From: Ranjit Manomohan @ 2006-05-15 21:19 UTC (permalink / raw)
  To: David S. Miller; +Cc: akpm, ranjitm, linux-kernel, netdev
In-Reply-To: <20060514.134231.101346572.davem@davemloft.net>

On Sun, 14 May 2006, David S. Miller wrote:

> From: Andrew Morton <akpm@osdl.org>
> Date: Sun, 14 May 2006 03:10:34 -0700
> 
> > It's a bit sad to be taking a clone of a clone like this.
> > Avoidable?
> 
> Besides, clones of clones are illegal, if it's already a clone
> you must make a copy.
> 

Heres a new version which does a copy instead of the clone to avoid
the double cloning issue.

-Thanks,
Ranjit

--- linux-2.6/net/sched/sch_generic.c	2006-05-10 12:34:52.000000000 -0700
+++ linux/net/sched/sch_generic.c	2006-05-15 13:49:09.000000000 -0700
@@ -136,8 +136,11 @@
 
 			if (!netif_queue_stopped(dev)) {
 				int ret;
+				struct sk_buff *skbc = NULL;
+				/* Copy the skb so that we can trace it after
+				 * a successful transmit. */
 				if (netdev_nit)
-					dev_queue_xmit_nit(skb, dev);
+					skbc = skb_copy(skb, GFP_ATOMIC);
 
 				ret = dev->hard_start_xmit(skb, dev);
 				if (ret == NETDEV_TX_OK) { 
@@ -145,9 +148,20 @@
 						dev->xmit_lock_owner = -1;
 						spin_unlock(&dev->xmit_lock);
 					}
+					if (skbc) {
+						/* transmit succeeded, 
+						 * trace the copy. */
+						dev_queue_xmit_nit(skbc,dev);
+						kfree_skb(skbc);
+					}
 					spin_lock(&dev->queue_lock);
 					return -1;
 				}
+
+				/* Free copy if it exists */
+				if (skbc)
+					kfree_skb(skbc);
+
 				if (ret == NETDEV_TX_LOCKED && nolock) {
 					spin_lock(&dev->queue_lock);
 					goto collision; 
 

^ permalink raw reply

* Re: Patches from Marcin Juszkiewicz
From: Marcin Juszkiewicz @ 2006-05-15 21:00 UTC (permalink / raw)
  To: netdev; +Cc: John W. Linville, Pavel Roskin, David Gibson
In-Reply-To: <20060515200616.GB424@tuxdriver.com>

Dnia poniedziałek, 15 maja 2006 22:06, John W. Linville napisał:
> On Fri, May 12, 2006 at 04:43:30PM -0400, Pavel Roskin wrote:
> > As a co-maintainer of Orinoco driver, I'd like to ask the netdev team
> > not to apply any patch from Marcin Juszkiewicz touching the Orinoco
> > driver without my or David's explicit approval.
>
> I plan to honor this request.  

> Are there any objections? 

Not from my side - I updated all hostap patches to not touch orinoco 
driver and can update them to be applied to current netdev git (without 
Pavel Roskin 24_hostap_cs_id.diff patch).

-- 
JID: hrw-jabber.org
Palmtop: Sharp Zaurus C760
OpenEmbedded/OpenZaurus developer

                 punk's not dead. it just smells that way.

^ permalink raw reply

* Re: SIOCSIWESSID + SIOCSIWAP behaviour
From: Jean Tourrilhes @ 2006-05-15 20:28 UTC (permalink / raw)
  To: Dan Williams; +Cc: Daniel Drake, Jouni Malinen, netdev, softmac-dev
In-Reply-To: <1147663779.2204.27.camel@localhost.localdomain>

On Sun, May 14, 2006 at 11:29:38PM -0400, Dan Williams wrote:
> On Mon, 2006-05-15 at 00:29 +0100, Daniel Drake wrote:
> > Hi Jean,

	Hi,

	Nice discussion you got going here ;-)

> > I'd just like to check my understanding (and softmacs implementation) 
> > of SIWESSID and SIWAP behaviour, for managed mode.
> > 
> > When SIWESSID happens, softmac drops association/authentication with the 
> > current network and then starts a scan for the requested SSID. When 
> > found, softmac authenticates and associates to that network.
> > 
> > When SIWAP happens, softmac drops association/authentication with the 
> > current network and then starts a scan for the requested BSSID. When 
> > found, softmac authenticates and associates to that network.
> 
> I'll chime in too... My understanding of WE is that your understanding
> of WE is correct.

	Yes.
	There is one case where you don't need to reassociate : if you
already have the correct ESSID or BSSID. This mostly happens when you
set the ESSID to any or the BSSID to off or any.
	Also, Jouni is correct that you don't need to scan if your
scan results are fresh enough.

>  But if the user has already set the SSID and _then_
> sets the BSSID, the driver must attempt to associate with an AP that has
> _both_ those properties.  Setting one doesn't cancel the other out
> (Jean, correct if I'm wrong here?)

	Correct.

>  AFAIK, you can actually set any old
> BSSID you like on the access point, so there's no guarantee that the
> BSSID any access point advertises is unique.  Furthermore I believe you
> can have one AP with one BSSID server more than one ESSID.  Higher-end
> Cisco/3Com/etc products allow this.

	I believe the BSSID has to be unique. HP APs can also offer
multiple ESSID for the same BSSID, but they do so using different
BSSID. If you look at the 802.11 spec, I can't see how two different
virtual cells can have the same BSSID, because the BSSID is the only
thing that identify the cell (the ESSID is not in each packet).

> > Is it correct that SIWAP can legally select *any* AP? (As opposed to 
> > only being for selecting a specific AP *on the ESS* indicated by a past 
> > or future SIWESSID call)
> 
> Correct.  The user may SIWAP _any_ BSSID at all, not necessarily related
> to the SSID.  However, if the user just set an ESSID with SIWESSID, I'm
> fairly sure that ESSID must be honored as well.

	The main configuration is SIWESSID, that's the only config
that is required by the standard. WIWAP is only a secondary
configuration, that is optional and not supported in all drivers. The
ESSID set should always be respected.
	If an ESSID is set, SIWAP should only look within APs having
this ESSID, and not use a different ESSID. Only if the ESSID is set to
'any' should the card really pick any AP regardless of ESSID.
	Personally, I think that SIWAP is too tricky to use properly
for most users, so we should encourage them to not use it.

> > No matter how I think of it, this strikes me as a hard interface to 
> > implement. Because association isn't an atomic operation, it is tricky 
> > to get the sequencing right, e.g. if the user does SIWESSID to start 
> > association, and then SIWAP to select a different AP before the original 
> > association has completed.
> 
> Hmm, what problems does this have?  It's not really any different than
> if the user issues two SIWESSID calls in short sequence, so you have to
> handle the start assoc->disassoc->start assoc sequence anyway...

	I agree that it make things messy in the driver. I also agree
that the user can do stupid things with iwconfig, and therefore there
is no real difference.
	There is still a performance issue. Having to cancel and
restart associations waste CPU cycles. One way to deal with that would
be to improve the "commit" strategy of the Wireless API. Basically,
every time you do a set, you schedule or reschedule the "commit" ioctl
to happen in a few dozen ms. This way, you could bundle all the
settting of apps such as wpa_supplicant with only a single
commit. This is similar to the way the routing API works.
	But, you won't get away that end-users can do stupid stuff
with iwconfig.

> Dan

	Have fun...

	Jean

^ permalink raw reply

* Re: Hardware button support for Wireless cards
From: John W. Linville @ 2006-05-15 20:08 UTC (permalink / raw)
  To: Jason Lunz; +Cc: netdev
In-Reply-To: <e4a6lq$g5v$1@sea.gmane.org>

On Mon, May 15, 2006 at 03:27:54PM +0000, Jason Lunz wrote:

> So if there's any desire for consistency of wireless button support,
> then I think the software-controlled ones should always shut off the
> radio, and optionally inform userspace or the wireless stack using some
> event mechanism.

FWIW, I think this position makes the most sense...

John
-- 
John W. Linville
linville@tuxdriver.com

^ permalink raw reply

* Re: iptables broken on ppc (ptrace too?) (2.6.17-rc3)
From: Meelis Roos @ 2006-05-15 20:07 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netdev
In-Reply-To: <4468D823.5090802@trash.net>

> Ah OK, I thought it was related to the new compat code, but that isn't
> the case. Your trace doesn't give much clues, except that it shows that
> iptables dies with a SIGSEGV, which might be a bug in userspace or in
> the kernel. Which was the last kernel that worked for you, and can you
> try that version again to rule out userspace bugs?

It's a fresh install in the new role with a new HDD and 2 NICs for a 
firewall so I have not tried earlier kernels with iptables. I do have 
sarge's 2.6.8 also working on that machine but not the kernels from 
testing and unstable (2.5.15 and 2.6.16) because the lack of prep 
support at the moment. I could problably compile some intermediate 
kernels but I haven't tested even the working 2.6.8 first with iptables 
yet.

I will be away tomorrow but can test some kernels the day after 
tomorrow. Only some of them because it's not a fast compile engine.

It only SIGSEGVs when ptraced and just gets Invalid Argument errors when 
not traced so this SEGV may be something different (perhaps ptrace 
related).

-- 
Meelis Roos (mroos@linux.ee)

^ permalink raw reply

* Re: Patches from Marcin Juszkiewicz
From: John W. Linville @ 2006-05-15 20:06 UTC (permalink / raw)
  To: Pavel Roskin; +Cc: netdev, David Gibson, Marcin Juszkiewicz
In-Reply-To: <1147466610.8478.41.camel@dv>

On Fri, May 12, 2006 at 04:43:30PM -0400, Pavel Roskin wrote:

> As a co-maintainer of Orinoco driver, I'd like to ask the netdev team
> not to apply any patch from Marcin Juszkiewicz touching the Orinoco
> driver without my or David's explicit approval.

I plan to honor this request.  Are there any objections?

John
-- 
John W. Linville
linville@tuxdriver.com

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox