* Re: [patch,rfc] allow registration of multiple netpolls per interface
From: Matt Mackall @ 2005-06-22 17:01 UTC (permalink / raw)
To: Jeff Moyer; +Cc: netdev, netdev, linux-kernel
In-Reply-To: <17081.20441.714191.528270@segfault.boston.redhat.com>
On Wed, Jun 22, 2005 at 07:47:37AM -0400, Jeff Moyer wrote:
> mpm> Hmm. It's conceivable we'll want netdump and kgdb on the same
> mpm> interface so we'll have to address this eventually..
>
> Well, do you want to address it eventually, or now? As I said, it's never
> bitten anyone before.
Later's fine. I just don't want to design it out by accident again.
> >> struct netpoll_info {
> >> spinlock_t poll_lock;
> >> int poll_owner;
> >> int rx_flags;
> >> spinlock_t rx_lock;
> >> struct netpoll *rx_np; /* netpoll that registered an rx_hook */
> >> };
> >>
> >> This is the structure which gets pointed to by the net_device. All of the
> >> flags and locks which are specific to the INTERFACE go here. Any variables
> >> which must be kept per struct netpoll were left in the struct netpoll. So
> >> now, we have a cleaner separation of data and its scope.
> >>
> >> Since we never really supported having more than one struct netpoll
> >> register an rx_hook, I got rid of the rx_list. This is replaced by a
> >> single pointer in the netpoll_info structure (np_rx). We still need to
> >> protect addition or removal of the rx_np pointer, and so keep the lock
> >> (rx_lock). There is one lock per struct net_device, and I am certain that
> >> it will be 0 contention, as rx_np will only be changed during an insmod or
> >> rmmod. If people think this would be a good rcu candidate, let me know and
> >> I'll change it to use that locking scheme.
>
> mpm> It might be simpler to have a single lock here..?
>
> Maybe. You can't really have netpoll code running on multiple cpus at the
> same time, right? This is the rx path, remember, so the other cpu should
> be spinning on the poll_lock.
>
> Keeping separate locks would allow you to unregister a struct netpoll
> associated with another net device without causing lock contention. This
> is a very minor win, obviously.
>
> I still feel like this npinfo struct is the right place for this, though.
> If you're strongly opposed to that, I'll change it.
No, certainly having it in npinfo makes sense. I just was wondering if
we really need two locks in there.
> >> + spin_lock_irqsave(&npinfo->rx_lock, flags);
> >> + if (npinfo->rx_np->dev == skb->dev)
> >> + np = npinfo->rx_np;
> >> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
>
> mpm> And I think that means we don't need the lock here either.
>
> Sure we do. We need to protect against rmmod's.
How can we have an rmmmod when we're trapped?
> >> static inline int netpoll_rx(struct sk_buff *skb)
> >> {
> >> - return skb->dev->np && skb->dev->np->rx_flags && __netpoll_rx(skb);
> >> + struct netpoll_info *npinfo = skb->dev->npinfo;
> >> + unsigned long flags;
> >> + int ret = 0;
> >> +
> >> + if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
> >> + return 0;
> >> +
> >> + spin_lock_irqsave(&npinfo->rx_lock, flags);
> >> + /* check rx_flags again with the lock held */
> >> + if (npinfo->rx_flags && __netpoll_rx(skb))
> >> + ret = 1;
> >> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> >> +
> >> + return ret;
> >> }
>
> mpm> This is perhaps a problem due to cache line bouncing. Perhaps we can
> mpm> use an atomic op and a memory barrier instead?
>
> It really should be a 0 contention lock. Let's not optimize something that
> doesn't need it. If we find that it causes problems, I'll be more than
> happy to fix it.
Ok, fair enough.
--
Mathematics is the supreme nostalgia of our time.
^ permalink raw reply
* Re: [PATCH] tcp: efficient port randomistion (rev 3)
From: Stephen Hemminger @ 2005-06-22 16:44 UTC (permalink / raw)
To: Michael Vittrup Larsen; +Cc: David S. Miller, netdev
In-Reply-To: <200506221117.04334.michael.vittrup.larsen@ericsson.com>
On Wed, 22 Jun 2005 11:17:03 +0200
Michael Vittrup Larsen <michael.vittrup.larsen@ericsson.com> wrote:
> On Tuesday 21 December 2004 00:39, David S. Miller wrote:
> > On Fri, 10 Dec 2004 17:09:00 -0800
> >
> > Stephen Hemminger <shemminger@osdl.org> wrote:
> > > okay, here is the revised version. Testing shows that it
> > > is more consistent, and just as fast as existing code,
> > > probably because of the getting rid of portalloc_lock and
> > > better distribution.
> > >
> > > Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> >
> > Queued up for 2.6.11, thanks Stephen.
>
> What's the status of this - I see it is not part of 2.6.12?
>
> Is there a general dislike of the port randomisation mechanism or?
There is port randomization in 2.6.11 and 2.6.12, look for
secure_tcp_port_ephemeral in the source. 2.6.12 also does random port
allocation for IPV6.
We still do the non-random stuff for explicit binds (tcp_v4_get_port),
but there is no state to seed in that case and it only impacts app's
that do an explicit bind to 0.
^ permalink raw reply
* RE: RFC: NAPI packet weighting patch
From: jamal @ 2005-06-22 16:37 UTC (permalink / raw)
To: Leonid Grossman
Cc: 'Donald Becker', 'Andi Kleen',
'Rick Jones', netdev, davem
In-Reply-To: <200506221623.j5MGNkxS001340@guinness.s2io.com>
On Wed, 2005-22-06 at 09:23 -0700, Leonid Grossman wrote:
> >
> > See the comment above. We decide if a packet is multicast vs.
> > unicast, IP vs. other at approximately
> > interrupt/"rx_copybreak" time. Very few NIC provide this
> > info in status bits, so we end up looking at the packet
> > header. That read moves the previously known-uncached data
> > (after all, it was just came in from a bus write) into the L1
> > cache for the CPU handling the device. Once it's there, the
> > copy is almost free.
>
> What status bits a NIC has to provide, in order for the stack to avoid
> touching headers?
> In our case, the headers are separated by the hardware so ideally we would
> like to avoid any header processing altogether,
> and reduce the number of cache misses.
>
Provide metadata that can be used to totaly replace eth_type_trans()
i.e answer the questions: is it multi/uni/broadcast, is the packet for
us (you would need to be programmed with what for us means), Is it IP,
ARP etc. I am sure any standard NIC these days can do a subset of these
You want to go one step further then allow the user to download a
number of filters and tell you what tag you should put on the descriptor
when sending the packet to user space on a match or mismatch.
If say you allowed 1024 such filters (not very different from the
current multicast filters), you could cut down a lot of CPU time.
cheers,
jamal
^ permalink raw reply
* RE: RFC: NAPI packet weighting patch
From: Leonid Grossman @ 2005-06-22 16:23 UTC (permalink / raw)
To: 'Donald Becker', 'Andi Kleen'
Cc: 'Rick Jones', netdev, davem
In-Reply-To: <Pine.LNX.4.44.0506211642290.21812-100000@bluewest.scyld.com>
>
> See the comment above. We decide if a packet is multicast vs.
> unicast, IP vs. other at approximately
> interrupt/"rx_copybreak" time. Very few NIC provide this
> info in status bits, so we end up looking at the packet
> header. That read moves the previously known-uncached data
> (after all, it was just came in from a bus write) into the L1
> cache for the CPU handling the device. Once it's there, the
> copy is almost free.
What status bits a NIC has to provide, in order for the stack to avoid
touching headers?
In our case, the headers are separated by the hardware so ideally we would
like to avoid any header processing altogether,
and reduce the number of cache misses.
>
> [[ Background: Yes, the allocating the new skbuff is very
> expensive. But we can either allocate a new, correctly-sized
> skbuff to copy into, or allocate a new full-sized skbuff to
> replace the one we will send to the Rx queue. ]]
>
> > > - cold memory lines from PCI writes
> >
> > I suspect in '96 chipsets also didn't do as aggressive
> prefetching as
> > they do today.
>
> Prefetching helps linear read bandwidth, but we shouldn't be
> triggering it. And I claim that cache line prefetching only
> restores the relative balance between L1/L2 caches, otherwise
> the long L2 cache lines would be very expensive with
> bump-read-bump-read with linear scans through memory.
>
> --
> Donald Becker becker@scyld.com
> Scyld Software Scyld Beowulf
> cluster systems
> 914 Bay Ridge Road, Suite 220 www.scyld.com
> Annapolis MD 21403 410-990-9993
>
>
>
^ permalink raw reply
* [TG3]: About hw coalescing infrastructure.
From: Eric Dumazet @ 2005-06-22 15:25 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, mchan
In-Reply-To: <20050511.141530.57445142.davem@davemloft.net>
David S. Miller a écrit :
> Ok, now that we have the tagged status stuff sorted I began
> to work on putting the hw mitigation bits back into the
> driver. The discussion on the DMA rw-ctrl settings is still
> ongoing, but I will get back to it shortly.
>
> This is the first step, we cache the settings in the tg3
> struct and put those values into the chip via tg3_set_coalesce().
>
> ETHTOOL_GCOALESCE is supported, setting is not.
>
Hi David
I am using 2.6.12 now, but still experiment a high number of interrupts per second on
my tg3 NIC, on an dual Opteron based machine.
(about 7300 interrupts per second generated by one interface eth0, 100Mbit/s link)
Is there anything I can try to tune the coalescing ?
Being able to handle 100 packets each interrupt instead of one or two would certainly help.
I dont mind about latency. But of course I would like not to drop packets :)
But maybe the BCM5702 is not able to delay an interrupt ?
Thank you
Eric Dumazet
----------------------------------------------------------------------------------------
# lspci -v
02:03.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5702 Gigabit Ethernet (rev 02)
Subsystem: Broadcom Corporation BCM5702 1000Base-T
Flags: bus master, 66Mhz, medium devsel, latency 64, IRQ 27
Memory at 00000000fe000000 (64-bit, non-prefetchable) [size=64K]
Expansion ROM at <unassigned> [disabled] [size=64K]
Capabilities: [40] PCI-X non-bridge device.
Capabilities: [48] Power Management version 2
Capabilities: [50] Vital Product Data
Capabilities: [58] Message Signalled Interrupts: 64bit+ Queue=0/3 Enable-
# ethtool -c eth0
Coalesce parameters for eth0:
Adaptive RX: off TX: off
stats-block-usecs: 1000000
sample-interval: 0
pkt-rate-low: 0
pkt-rate-high: 0
rx-usecs: 20
rx-frames: 5
rx-usecs-irq: 20
rx-frames-irq: 5
tx-usecs: 72
tx-frames: 53
tx-usecs-irq: 20
tx-frames-irq: 5
rx-usecs-low: 0
rx-frame-low: 0
tx-usecs-low: 0
tx-frame-low: 0
rx-usecs-high: 0
rx-frame-high: 0
tx-usecs-high: 0
tx-frame-high: 0
# ethtool -S eth0
NIC statistics:
rx_octets: 104634072366
rx_fragments: 0
rx_ucast_packets: 852685070
rx_mcast_packets: 0
rx_bcast_packets: 20429
rx_fcs_errors: 0
rx_align_errors: 0
rx_xon_pause_rcvd: 0
rx_xoff_pause_rcvd: 0
rx_mac_ctrl_rcvd: 0
rx_xoff_entered: 0
rx_frame_too_long_errors: 0
rx_jabbers: 0
rx_undersize_packets: 0
rx_in_length_errors: 0
rx_out_length_errors: 0
rx_64_or_less_octet_packets: 451956709
rx_65_to_127_octet_packets: 272058231
rx_128_to_255_octet_packets: 63364655
rx_256_to_511_octet_packets: 35814973
rx_512_to_1023_octet_packets: 11867701
rx_1024_to_1522_octet_packets: 17643210
rx_1523_to_2047_octet_packets: 0
rx_2048_to_4095_octet_packets: 0
rx_4096_to_8191_octet_packets: 0
rx_8192_to_9022_octet_packets: 0
tx_octets: 134640205605
tx_collisions: 0
tx_xon_sent: 0
tx_xoff_sent: 0
tx_flow_control: 0
tx_mac_errors: 0
tx_single_collisions: 0
tx_mult_collisions: 0
tx_deferred: 0
tx_excessive_collisions: 0
tx_late_collisions: 0
tx_collide_2times: 0
tx_collide_3times: 0
tx_collide_4times: 0
tx_collide_5times: 0
tx_collide_6times: 0
tx_collide_7times: 0
tx_collide_8times: 0
tx_collide_9times: 0
tx_collide_10times: 0
tx_collide_11times: 0
tx_collide_12times: 0
tx_collide_13times: 0
tx_collide_14times: 0
tx_collide_15times: 0
tx_ucast_packets: 774312055
tx_mcast_packets: 13
tx_bcast_packets: 246
tx_carrier_sense_errors: 0
tx_discards: 0
tx_errors: 0
dma_writeq_full: 21375
dma_write_prioq_full: 0
rxbds_empty: 0
rx_discards: 2644
rx_errors: 0
rx_threshold_hit: 57384403
dma_readq_full: 27100189
dma_read_prioq_full: 1557267
tx_comp_queue_full: 35712755
ring_set_send_prod_index: 747986769
ring_status_update: 502110997
nic_irqs: 446148615
nic_avoided_irqs: 55962382
nic_tx_threshold_hit: 37282069
# ethtool -g eth0
Ring parameters for eth0:
Pre-set maximums:
RX: 511
RX Mini: 0
RX Jumbo: 255
TX: 0
Current hardware settings:
RX: 200
RX Mini: 0
RX Jumbo: 100
TX: 511
# ethtool eth0
Settings for eth0:
Supported ports: [ MII ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Half 1000baseT/Full
Supports auto-negotiation: Yes
Advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Half 1000baseT/Full
Advertised auto-negotiation: Yes
Speed: 100Mb/s
Duplex: Full
Port: Twisted Pair
PHYAD: 1
Transceiver: internal
Auto-negotiation: on
Supports Wake-on: g
Wake-on: d
Current message level: 0x000000ff (255)
Link detected: yes
# cat /proc/interrupts (HZ=200)
CPU0 CPU1
0: 164055 14038348 IO-APIC-edge timer
2: 0 0 XT-PIC cascade
8: 0 0 IO-APIC-edge rtc
14: 4073 368224 IO-APIC-edge ide0
15: 0 20 IO-APIC-edge ide1
27: 35985951 421578656 IO-APIC-level eth0, eth1
NMI: 874625217 905019517 (oprofile running)
LOC: 14201857 14201976
ERR: 0
MIS: 0
oprofile data :
# more /tmp/vmlinux.oprofile
CPU: Hammer, speed 2205.08 MHz (estimated)
Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit mask of 0x00 (No unit mask) count 100000
samples cum. samples % cum. % symbol name
20208503 20208503 7.7982 7.7982 ipt_do_table
8336463 28544966 3.2169 11.0151 tcp_v4_rcv
7746814 36291780 2.9894 14.0045 handle_IRQ_event
7117968 43409748 2.7467 16.7512 tg3_poll
6585377 49995125 2.5412 19.2924 memcpy
5184695 55179820 2.0007 21.2931 ip_route_input
4346890 59526710 1.6774 22.9705 kfree
4214007 63740717 1.6261 24.5967 copy_user_generic_c
4093885 67834602 1.5798 26.1764 tcp_ack
4006753 71841355 1.5462 27.7226 tg3_interrupt_tagged
3778976 75620331 1.4583 29.1809 tcp_rcv_established
3756498 79376829 1.4496 30.6304 ip_queue_xmit
3418999 82795828 1.3193 31.9498 schedule
3274459 86070287 1.2636 33.2134 try_to_wake_up
3034809 89105096 1.1711 34.3844 tcp_sendmsg
2846436 91951532 1.0984 35.4828 kmem_cache_alloc
2745147 94696679 1.0593 36.5422 free_block
2679056 97375735 1.0338 37.5760 kmem_cache_free
2595289 99971024 1.0015 38.5775 fn_hash_lookup
2582072 102553096 0.9964 39.5738 __memset
2576462 105129558 0.9942 40.5681 tcp_transmit_skb
2528313 107657871 0.9756 41.5437 tcp_recvmsg
2392370 110050241 0.9232 42.4669 timer_interrupt
2365615 112415856 0.9129 43.3797 system_call
2358666 114774522 0.9102 44.2899 sockfd_lookup
2357192 117131714 0.9096 45.1995 tcp_poll
2340568 119472282 0.9032 46.1027 ip_rcv
2315805 121788087 0.8936 46.9964 tcp_match
2276212 124064299 0.8784 47.8747 sys_epoll_wait
2260913 126325212 0.8725 48.7472 __mod_timer
2173905 128499117 0.8389 49.5861 tg3_start_xmit
2057738 130556855 0.7941 50.3801 __switch_to
2022435 132579290 0.7804 51.1605 ep_poll_callback
2020449 134599739 0.7797 51.9402 sock_wfree
1913008 136512747 0.7382 52.6784 find_busiest_group
1891578 138404325 0.7299 53.4083 local_bh_enable
1860130 140264455 0.7178 54.1261 ip_local_deliver
1793639 142058094 0.6921 54.8183 __ip_route_output_key
1789287 143847381 0.6905 55.5087 alloc_skb
1770972 145618353 0.6834 56.1921 tcp_write_timer
1727286 147345639 0.6665 56.8587 __wake_up
1634111 148979750 0.6306 57.4893 skb_release_data
1625157 150604907 0.6271 58.1164 __kmalloc
1567198 152172105 0.6048 58.7211 tcp_v4_do_rcv
1562495 153734600 0.6029 59.3241 __kfree_skb
^ permalink raw reply
* Print one record only - addition
From: Tomáš Macek @ 2005-06-22 13:53 UTC (permalink / raw)
To: netdev
In-Reply-To: <20050618202359.GP22463@postel.suug.ch>
On Sat, 18 Jun 2005, Thomas Graf wrote:
> * Tom?? Macek <Pine.LNX.4.61.0506182042540.29813@localhost.localdomain> 2005-06-18 20:55
>> The 'rtm_dst_len = 16' should mean the mask of the route I'm looking for, correct?
>
> Yes.
>
>> The whole code before sending the packet is below:
>>
>>
>> /* Create Socket */
>> if((sock = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)) < 0)
>> perror("Socket Creation: ");
>>
>> /* Initialize the buffer */
>> memset(msgBuf, 0, BUFSIZE);
>>
>> /* point the header and the msg structure pointers into the buffer */
>> nlMsg = (struct nlmsghdr *)msgBuf;
>> rtMsg = (struct rtmsg *)NLMSG_DATA(nlMsg);
>> rtMsg->rtm_family = AF_INET;
>> rtMsg->rtm_dst_len = 16;
>>
>> /* Fill in the nlmsg header*/
>> nlMsg->nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)); // Length of message.
>> nlMsg->nlmsg_type = RTM_GETROUTE; // Get the routes from kernel routing table .
>> nlMsg->nlmsg_flags = NLM_F_REQUEST; // The message is a request for dump.
>> nlMsg->nlmsg_seq = msgSeq++; // Sequence of the message packet.
>> nlMsg->nlmsg_pid = getpid(); // PID of process sending the request.
>>
>> char *cp;
>> unsigned int xx[4]; int i = 0;
>> unsigned char *ap = (unsigned char *)xx;
>> for (cp = argv[1], i = 0; *cp; cp++) {
>> if (*cp <= '9' && *cp >= '0') {
>> ap[i] = 10*ap[i] + (*cp-'0');
>> continue;
>> }
>> if (*cp == '.' && ++i <= 3)
>> continue;
>> return -1;
>> }
>>
>> NetlinkAddAttr(nlMsg, sizeof(nlMsg), RTA_DST, &xx, 4);
>
> This looks good but your NetlinkAddAttr is bogus, it should
> be something like this:
>
> int nl_msg_append_tlv(struct nlmsghdr *n, int type, void *data, size_t len)
> {
> int tlen;
> struct rtattr *rta;
>
> tlen = NLMSG_ALIGN(n->nlmsg_len) + RTA_LENGTH(NLMSG_ALIGN(len));
>
> rta = (struct rtattr *) NLMSG_TAIL(n);
> rta->rta_type = type;
> rta->rta_len = RTA_LENGTH(NLMSG_ALIGN(len));
> memcpy(RTA_DATA(rta), data, len);
> n->nlmsg_len = tlen;
>
> return 0;
> }
>
> Your code is missing various alignment requirements. I can't tell
> whether this is the last bug. I recommend you to read ip/iproute.c
> in the iproute2 source or give libnl a second chance.
>
The code now works this way:
[root@localhost route]# route
1.1.1.0 * 255.255.255.0 U 0 0 0 eth0
3.3.0.0 * 255.255.0.0 U 0 0 0 eth1
default meric 0.0.0.0 UG 0 0 0 eth0
[root@localhost route]# ./a.out 2.2.2.2 16
Destination Gateway Interface Source Netmask
2.2.2.2 213.250.192.33 eth0 255.255.255.255
[root@localhost route]# ./a.out 1.1.1.2 16
Destination Gateway Interface Source Netmask
1.1.1.2 *.*.*.* eth0 255.255.255.255
[root@localhost route]# ./a.out 3.3.3.2 16
Destination Gateway Interface Source Netmask
3.3.3.2 *.*.*.* eth1 255.255.255.255
so it returns the route, where the data would go, if their DST address would be the one given by the argv[1] with mask argv[2].
I don't know now, if we understood to each other and if this is you thought it should be.
If I will write on the command line './a.out 3.3.0.0 16', it should print the line like this if the record is present:
3.3.0.0 * 255.255.0.0 U 0 0 0 eth1
if I would write './a.out 3.3.3.1 32' it MUST print nothing! :)
Thank you for help
^ permalink raw reply
* [PATCH] dont use strlen() but the result from a prior sprintf()
From: Eric Dumazet @ 2005-06-22 12:56 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
In-Reply-To: <20050621.165634.07642938.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 769 bytes --]
Hi David
Small patch to save an unecessary call to strlen() : sprintf() gave us the length, just trust it.
Thank you
Eric Dumazet
diff -Nu linux-2.6.12-orig/net/socket.c linux-2.6.12/net/socket.c
--- linux-2.6.12-orig/net/socket.c 2005-06-22 14:47:56.000000000 +0200
+++ linux-2.6.12/net/socket.c 2005-06-22 14:49:22.000000000 +0200
@@ -382,9 +382,8 @@
goto out;
}
- sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
+ this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
this.name = name;
- this.len = strlen(name);
this.hash = SOCK_INODE(sock)->i_ino;
file->f_dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);
[-- Attachment #2: patch.1 --]
[-- Type: text/plain, Size: 446 bytes --]
--- linux-2.6.12-orig/net/socket.c 2005-06-22 14:47:56.000000000 +0200
+++ linux-2.6.12/net/socket.c 2005-06-22 14:49:22.000000000 +0200
@@ -382,9 +382,8 @@
goto out;
}
- sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
+ this.len = sprintf(name, "[%lu]", SOCK_INODE(sock)->i_ino);
this.name = name;
- this.len = strlen(name);
this.hash = SOCK_INODE(sock)->i_ino;
file->f_dentry = d_alloc(sock_mnt->mnt_sb->s_root, &this);
^ permalink raw reply
* Re: [patch,rfc] allow registration of multiple netpolls per interface
From: Jeff Moyer @ 2005-06-22 11:47 UTC (permalink / raw)
To: Matt Mackall; +Cc: netdev, netdev, linux-kernel
In-Reply-To: <20050621225252.GY27572@waste.org>
==> Regarding Re: [patch,rfc] allow registration of multiple netpolls per interface; Matt Mackall <mpm@selenic.com> adds:
mpm> On Tue, Jun 21, 2005 at 05:41:34PM -0400, Jeff Moyer wrote:
>> Hi,
>>
>> This patch restores functionality that was removed when the recursive
-> poll bug was fixed. Namely, it allows multiple netpoll clients to
>> register against the same network interface.
mpm> Thanks. I've been neglecting this for a bit while I've been busy with
mpm> other things.
>> In order to put things into perspective, I'm going to provide some
>> background information. So, here is how things used to work:
>>
>> Multiple users of the netpoll interface could register themselves to send
>> packets over the same interface. Any number of these netpoll clients could
>> register an rx_hook, as well. However, only the very first in the list
>> (hence the last one that registered), that matched the incoming interface,
>> would be called when a packet arrived. The reason for this was not design,
>> it was an oversight in the implementation. In practice, however, no one
>> ever stumbled over this. (There are more subtleties when dealing with
>> multiple rx_hooks registered to the same interface, but we'll ignore these,
>> since no one ever ran into such problems.)
mpm> Hmm. It's conceivable we'll want netdump and kgdb on the same
mpm> interface so we'll have to address this eventually..
Well, do you want to address it eventually, or now? As I said, it's never
bitten anyone before.
>> Note that each netpoll client that registered an rx_hook was put on a
>> netpoll_rx_list. This list was protected by a spinlock, and so operations
>> which touched the rx routines would incur a locking penalty and a list
>> traversal. I am mentioning this because the list and associated lock were
>> removed when the code was refactored, and the patches I propose will
>> reintroduce the lock, but not the list.
mpm> ..so we'll probably want the list back in some form. Sigh.
>> Moving to what we have today:
>>
>> Multiple netpoll clients can register to send packets over the same
>> interface. That's right, you can actually do this. However, there are
>> ugly side effects. Because we now have a pointer from the net_device to a
>> struct netpoll, the last netpoll client to register will be pointed to by
>> the net_device->np. What this means is that if you had two clients, the
>> first registers an rx_hook and the second does not, then the netpoll code
>> will not know that any device has actually registered an rx_hook (since the
>> np pointer in the struct net_device is overwritten)! As a result, no
>> incoming packets will be delivered to the registered rx routine. This is
>> clearly undesirable behaviour.
>>
>> So what does the patch do?
>>
>> I created a new structure:
>>
>> struct netpoll_info {
>> spinlock_t poll_lock;
>> int poll_owner;
>> int rx_flags;
>> spinlock_t rx_lock;
>> struct netpoll *rx_np; /* netpoll that registered an rx_hook */
>> };
>>
>> This is the structure which gets pointed to by the net_device. All of the
>> flags and locks which are specific to the INTERFACE go here. Any variables
>> which must be kept per struct netpoll were left in the struct netpoll. So
>> now, we have a cleaner separation of data and its scope.
>>
>> Since we never really supported having more than one struct netpoll
>> register an rx_hook, I got rid of the rx_list. This is replaced by a
>> single pointer in the netpoll_info structure (np_rx). We still need to
>> protect addition or removal of the rx_np pointer, and so keep the lock
>> (rx_lock). There is one lock per struct net_device, and I am certain that
>> it will be 0 contention, as rx_np will only be changed during an insmod or
>> rmmod. If people think this would be a good rcu candidate, let me know and
>> I'll change it to use that locking scheme.
mpm> It might be simpler to have a single lock here..?
Maybe. You can't really have netpoll code running on multiple cpus at the
same time, right? This is the rx path, remember, so the other cpu should
be spinning on the poll_lock.
Keeping separate locks would allow you to unregister a struct netpoll
associated with another net device without causing lock contention. This
is a very minor win, obviously.
I still feel like this npinfo struct is the right place for this, though.
If you're strongly opposed to that, I'll change it.
>> In the process of making these changes, I've fixed a couple other minor
>> bugs [1]. These fixes are included in this patch, but I will break them
>> out if people agree with this approach.
>>
>> I have tested this by registering multiple netpoll clients, and verifying
>> that they both function properly. I have not yet tried registering an
>> rx_hook, but I believe the code should be sufficient to handle that case.
>>
>> And so, here is the full patch. I'd appreciate comments. Once we've
>> reached consensus, I will resubmit as a patch series.
mpm> I think the general idea is sound. So let's take a look at the patch itself.
>> Oh, and I've cc'd both netdev@oss.sgi.com and @vger.kernel.org. Is it safe
>> to just use the vger list?
mpm> Yes.
>> [1] netpoll_poll_unlock unlocked and then set the poll_owner. I've
>> reversed the order of those operations. The netpoll_cleanup code could
>> dereference a null pointer, that was fixed by virtue of being very
>> different in the new case.
mpm> Ok, let's fix the lock ordering bit first.
>> --- linux-2.6.12-rc6/net/core/netpoll.c.orig 2005-06-20 19:51:56.000000000 -0400
>> +++ linux-2.6.12-rc6/net/core/netpoll.c 2005-06-21 16:03:22.409620400 -0400
>> @@ -131,18 +131,19 @@ static int checksum_udp(struct sk_buff *
>> static void poll_napi(struct netpoll *np)
>> {
>> int budget = 16;
>> + struct netpoll_info *npinfo = np->dev->npinfo;
mpm> As a minor point of style, I like to put the "get my private info"
mpm> lines first.
Quite the minor nit! It's the second line in the function! I'll fix it,
though. ;)
>> @@ -245,6 +246,7 @@ repeat:
>> static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
>> {
>> int status;
>> + struct netpoll_info *npinfo;
>>
>> repeat:
>> if(!np || !np->dev || !netif_running(np->dev)) {
>> @@ -253,7 +255,8 @@ repeat:
>> }
>>
>> /* avoid recursion */
>> - if(np->poll_owner == smp_processor_id() ||
>> + npinfo = np->dev->npinfo;
mpm> Again, the npinfo assignment ought to happen as soon as possible.
This is as soon as possible. Note that above we check to see if np and
np->dev are valid pointers. We can't get to the npinfo struct before we
know that.
>> + if(npinfo->poll_owner == smp_processor_id() ||
np-> dev->xmit_lock_owner == smp_processor_id()) {
>> if (np->drop)
np-> drop(skb);
>> @@ -346,7 +349,15 @@ static void arp_reply(struct sk_buff *sk
>> int size, type = ARPOP_REPLY, ptype = ETH_P_ARP;
>> u32 sip, tip;
>> struct sk_buff *send_skb;
>> - struct netpoll *np = skb->dev->np;
>> + struct netpoll *np;
>> + struct netpoll_info *npinfo = skb->dev->npinfo;
>> +
>> + if (!npinfo) return;
mpm> We should only be replying to ARPs if we're trapped, right? How do we
mpm> get here with npinfo unset?
Good point.
mpm> The return ought to be on a separate line, btw.
Agreed.
>> + spin_lock_irqsave(&npinfo->rx_lock, flags);
>> + if (npinfo->rx_np->dev == skb->dev)
>> + np = npinfo->rx_np;
>> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
mpm> And I think that means we don't need the lock here either.
Sure we do. We need to protect against rmmod's.
>> if (!np) return;
mpm> And the same question and style criticism of my own code.
;)
>> @@ -429,9 +440,9 @@ int __netpoll_rx(struct sk_buff *skb)
>> int proto, len, ulen;
>> struct iphdr *iph;
>> struct udphdr *uh;
>> - struct netpoll *np = skb->dev->np;
>> + struct netpoll *np = skb->dev->npinfo->rx_np;
>>
>> - if (!np->rx_hook)
>> + if (!np)
>> goto out;
>> if (skb->dev->type != ARPHRD_ETHER)
>> goto out;
>> @@ -611,9 +622,8 @@ int netpoll_setup(struct netpoll *np)
>> {
>> struct net_device *ndev = NULL;
>> struct in_device *in_dev;
>> -
>> - np->poll_lock = SPIN_LOCK_UNLOCKED;
>> - np->poll_owner = -1;
>> + struct netpoll_info *npinfo;
>> + unsigned long flags;
>>
>> if (np->dev_name)
>> ndev = dev_get_by_name(np->dev_name);
>> @@ -624,7 +634,17 @@ int netpoll_setup(struct netpoll *np)
>> }
>>
np-> dev = ndev;
>> - ndev->np = np;
>> + if (!ndev->npinfo) {
>> + npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
>> + if (!npinfo)
>> + goto release;
>> +
>> + npinfo->rx_np = NULL;
>> + npinfo->poll_lock = SPIN_LOCK_UNLOCKED;
>> + npinfo->poll_owner = -1;
>> + npinfo->rx_lock = SPIN_LOCK_UNLOCKED;
>> + } else
>> + npinfo = ndev->npinfo;
>>
>> if (!ndev->poll_controller) {
>> printk(KERN_ERR "%s: %s doesn't support polling, aborting.\n",
>> @@ -692,13 +712,20 @@ int netpoll_setup(struct netpoll *np)
np-> name, HIPQUAD(np->local_ip));
>> }
>>
>> - if(np->rx_hook)
>> - np->rx_flags = NETPOLL_RX_ENABLED;
>> + if(np->rx_hook) {
>> + spin_lock_irqsave(&npinfo->rx_lock, flags);
>> + npinfo->rx_flags |= NETPOLL_RX_ENABLED;
>> + npinfo->rx_np = np;
>> + spin_unlock_irqsave(&npinfo->rx_lock, flags);
>> + }
>> + /* last thing to do is link it to the net device structure */
>> + ndev->npinfo = npinfo;
>>
>> return 0;
>>
>> release:
>> - ndev->np = NULL;
>> + if (!ndev->npinfo)
>> + kfree(npinfo);
np-> dev = NULL;
>> dev_put(ndev);
>> return -1;
>> @@ -706,9 +733,17 @@ int netpoll_setup(struct netpoll *np)
>>
>> void netpoll_cleanup(struct netpoll *np)
>> {
>> - if (np->dev)
>> - np->dev->np = NULL;
>> - dev_put(np->dev);
>> + struct netpoll_info *npinfo;
>> +
>> + if (np->dev) {
>> + npinfo = np->dev->npinfo;
>> + if (npinfo && npinfo->rx_np == np) {
>> + npinfo->rx_np = NULL;
>> + npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
>> + }
>> + dev_put(np->dev);
>> + }
>> +
np-> dev = NULL;
>> }
>>
>> --- linux-2.6.12-rc6/net/core/dev.c.orig 2005-06-20 19:51:59.000000000 -0400
>> +++ linux-2.6.12-rc6/net/core/dev.c 2005-06-21 13:53:51.583407710 -0400
>> @@ -1656,6 +1656,7 @@ int netif_receive_skb(struct sk_buff *sk
>> unsigned short type;
>>
>> /* if we've gotten here through NAPI, check netpoll */
>> + /* how else can we get here? --phro */
mpm> We can get here in the usual route of non-NAPI delivery, IIRC.
I couldn't find that path. I'll look again.
>> if (skb->dev->poll && netpoll_rx(skb))
>> return NET_RX_DROP;
>>
>> --- linux-2.6.12-rc6/include/linux/netpoll.h.orig 2005-06-20 19:51:47.000000000 -0400
>> +++ linux-2.6.12-rc6/include/linux/netpoll.h 2005-06-21 15:29:48.994422229 -0400
>> @@ -16,14 +16,19 @@ struct netpoll;
>> struct netpoll {
>> struct net_device *dev;
>> char dev_name[16], *name;
>> - int rx_flags;
>> void (*rx_hook)(struct netpoll *, int, char *, int);
>> void (*drop)(struct sk_buff *skb);
>> u32 local_ip, remote_ip;
>> u16 local_port, remote_port;
>> unsigned char local_mac[6], remote_mac[6];
>> +};
>> +
>> +struct netpoll_info {
>> spinlock_t poll_lock;
>> int poll_owner;
>> + int rx_flags;
>> + spinlock_t rx_lock;
>> + struct netpoll *rx_np; /* netpoll that registered an rx_hook */
>> };
>>
>> void netpoll_poll(struct netpoll *np);
>> @@ -39,22 +44,35 @@ void netpoll_queue(struct sk_buff *skb);
>> #ifdef CONFIG_NETPOLL
>> static inline int netpoll_rx(struct sk_buff *skb)
>> {
>> - return skb->dev->np && skb->dev->np->rx_flags && __netpoll_rx(skb);
>> + struct netpoll_info *npinfo = skb->dev->npinfo;
>> + unsigned long flags;
>> + int ret = 0;
>> +
>> + if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
>> + return 0;
>> +
>> + spin_lock_irqsave(&npinfo->rx_lock, flags);
>> + /* check rx_flags again with the lock held */
>> + if (npinfo->rx_flags && __netpoll_rx(skb))
>> + ret = 1;
>> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
>> +
>> + return ret;
>> }
mpm> This is perhaps a problem due to cache line bouncing. Perhaps we can
mpm> use an atomic op and a memory barrier instead?
It really should be a 0 contention lock. Let's not optimize something that
doesn't need it. If we find that it causes problems, I'll be more than
happy to fix it.
Thanks for the review, Matt. I'll put together another patch, test it, and
repost later today.
-Jeff
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-22 11:31 UTC (permalink / raw)
To: Chris Friesen; +Cc: Donald Becker, Andi Kleen, Rick Jones, netdev, davem
In-Reply-To: <42B8ECA0.5060904@nortel.com>
> If I recall, G4 chips are 32 bytes, and G5s are 128 bytes. Most current
> x86 chips are 64 bytes though.
P4s are effectively 128 byte. And that is the most common x86 right now.
-Andi
^ permalink raw reply
* Re: receive only one record from the routing table
From: Tomáš Macek @ 2005-06-22 10:07 UTC (permalink / raw)
To: netdev
In-Reply-To: <20050618202359.GP22463@postel.suug.ch>
>> ...or give libnl a second chance
I would like to give it second change, but when typing 'make', it outputs this:
...
Entering lib
MAKE libnl.so.0.5.1
CC helpers.c
helpers.c:417: error: `ARPHRD_EUI64' undeclared here (not in a function)
helpers.c:417: error: initializer element is not constant
helpers.c:417: error: (near initialization for `llprotos[13].i')
helpers.c:417: error: initializer element is not constant
helpers.c:417: error: (near initialization for `llprotos[13]')
helpers.c:418: error: initializer element is not constant
helpers.c:418: error: (near initialization for `llprotos[14]')
helpers.c:419: error: initializer element is not constant
helpers.c:419: error: (near initialization for `llprotos[15]')
helpers.c:420: error: initializer element is not constant
helpers.c:420: error: (near initialization for `llprotos[16]')
helpers.c:421: error: initializer element is not constant
helpers.c:421: error: (near initialization for `llprotos[17]')
...
On Sat, 18 Jun 2005, Thomas Graf wrote:
> * Tom?? Macek <Pine.LNX.4.61.0506182042540.29813@localhost.localdomain> 2005-06-18 20:55
>> The 'rtm_dst_len = 16' should mean the mask of the route I'm looking for, correct?
>
> Yes.
>
>> The whole code before sending the packet is below:
>>
>>
>> /* Create Socket */
>> if((sock = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)) < 0)
>> perror("Socket Creation: ");
>>
>> /* Initialize the buffer */
>> memset(msgBuf, 0, BUFSIZE);
>>
>> /* point the header and the msg structure pointers into the buffer */
>> nlMsg = (struct nlmsghdr *)msgBuf;
>> rtMsg = (struct rtmsg *)NLMSG_DATA(nlMsg);
>> rtMsg->rtm_family = AF_INET;
>> rtMsg->rtm_dst_len = 16;
>>
>> /* Fill in the nlmsg header*/
>> nlMsg->nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)); // Length of message.
>> nlMsg->nlmsg_type = RTM_GETROUTE; // Get the routes from kernel routing table .
>> nlMsg->nlmsg_flags = NLM_F_REQUEST; // The message is a request for dump.
>> nlMsg->nlmsg_seq = msgSeq++; // Sequence of the message packet.
>> nlMsg->nlmsg_pid = getpid(); // PID of process sending the request.
>>
>> char *cp;
>> unsigned int xx[4]; int i = 0;
>> unsigned char *ap = (unsigned char *)xx;
>> for (cp = argv[1], i = 0; *cp; cp++) {
>> if (*cp <= '9' && *cp >= '0') {
>> ap[i] = 10*ap[i] + (*cp-'0');
>> continue;
>> }
>> if (*cp == '.' && ++i <= 3)
>> continue;
>> return -1;
>> }
>>
>> NetlinkAddAttr(nlMsg, sizeof(nlMsg), RTA_DST, &xx, 4);
>
> This looks good but your NetlinkAddAttr is bogus, it should
> be something like this:
>
> int nl_msg_append_tlv(struct nlmsghdr *n, int type, void *data, size_t len)
> {
> int tlen;
> struct rtattr *rta;
>
> tlen = NLMSG_ALIGN(n->nlmsg_len) + RTA_LENGTH(NLMSG_ALIGN(len));
>
> rta = (struct rtattr *) NLMSG_TAIL(n);
> rta->rta_type = type;
> rta->rta_len = RTA_LENGTH(NLMSG_ALIGN(len));
> memcpy(RTA_DATA(rta), data, len);
> n->nlmsg_len = tlen;
>
> return 0;
> }
>
> Your code is missing various alignment requirements. I can't tell
> whether this is the last bug. I recommend you to read ip/iproute.c
> in the iproute2 source or give libnl a second chance.
>
>
>
>
>
>
^ permalink raw reply
* [2.6 patch] document that 8139TOO supports 8129/8130
From: Adrian Bunk @ 2005-06-22 9:24 UTC (permalink / raw)
To: Jeff Garzik; +Cc: netdev, linux-kernel
The 8129/8130 support is a sub-option that is not visible if the user
hasn't enabled the 8139 support.
Let's make it a bit easier for users to find the driver for their nic.
Signed-off-by: Adrian Bunk <bunk@stusta.de>
---
This patch was already sent on:
- 16 May 2005
--- linux-2.6.12-rc4-mm1-full/drivers/net/Kconfig.old 2005-05-13 05:54:05.000000000 +0200
+++ linux-2.6.12-rc4-mm1-full/drivers/net/Kconfig 2005-05-13 06:00:21.000000000 +0200
@@ -1484,14 +1484,14 @@
will be called 8139cp. This is recommended.
config 8139TOO
- tristate "RealTek RTL-8139 PCI Fast Ethernet Adapter support"
+ tristate "RealTek RTL-8129/8130/8139 PCI Fast Ethernet Adapter support"
depends on NET_PCI && PCI
select CRC32
select MII
---help---
This is a driver for the Fast Ethernet PCI network cards based on
- the RTL8139 chips. If you have one of those, say Y and read
- the Ethernet-HOWTO <http://www.tldp.org/docs.html#howto>.
+ the RTL 8129/8130/8139 chips. If you have one of those, say Y and
+ read the Ethernet-HOWTO <http://www.tldp.org/docs.html#howto>.
To compile this driver as a module, choose M here: the module
will be called 8139too. This is recommended.
^ permalink raw reply
* Re: [PATCH] tcp: efficient port randomistion (rev 3)
From: Michael Vittrup Larsen @ 2005-06-22 9:17 UTC (permalink / raw)
To: David S. Miller; +Cc: Stephen Hemminger, netdev
In-Reply-To: <20041220153916.6c00c114.davem@davemloft.net>
On Tuesday 21 December 2004 00:39, David S. Miller wrote:
> On Fri, 10 Dec 2004 17:09:00 -0800
>
> Stephen Hemminger <shemminger@osdl.org> wrote:
> > okay, here is the revised version. Testing shows that it
> > is more consistent, and just as fast as existing code,
> > probably because of the getting rid of portalloc_lock and
> > better distribution.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
>
> Queued up for 2.6.11, thanks Stephen.
What's the status of this - I see it is not part of 2.6.12?
Is there a general dislike of the port randomisation mechanism or?
/Michael
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: P @ 2005-06-22 8:42 UTC (permalink / raw)
To: David S. Miller
Cc: gandalf, hadi, shemminger, mitch.a.williams, john.ronciak, mchan,
buytenh, jdmason, netdev, Robert.Olsson, ganesh.venkatesan,
jesse.brandeburg
In-Reply-To: <20050621.133704.08321534.davem@davemloft.net>
David S. Miller wrote:
> Also, e1000 sends full MTU sized SKBs down into the stack even if the
> packet is very small. This also hurts performance a lot. As
> discussed elsewhere, it should use a "small packet" cut-off just like
> other drivers do. If the RX frame is less than this cut-off value, a
> new smaller sized SKB is allocated and the RX data copied into it.
> The RX ring SKB is left in-place and given back to the chip.
Yes the copy is essentially free here as the data is already cached.
As a data point, I went the whole hog and used buffer recycling
in my essentially packet sniffing application. I.E. there are no
allocs per packet at all, and this make a HUGE difference. On a
2x3.4GHz 2xe1000 system I can receive 620Kpps per port sustained
into my userspace app which does a LOT of processing per packet.
Without the buffer recycling is was around 250Kpps.
Note I don't reuse an skb until the packet is copied into a
PACKET_MMAP buffer.
--
Pádraig Brady - http://www.pixelbeat.org
--
^ permalink raw reply
* [PATCH] Micro optimization in eth_header()
From: Denis Vlasenko @ 2005-06-22 8:31 UTC (permalink / raw)
To: davem; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 28 bytes --]
Compile tested only.
--
vda
[-- Attachment #2: eth.c.diff --]
[-- Type: text/x-diff, Size: 2557 bytes --]
Micro optimization in eth_header().
Changes in asm code (new on the right):
...
50: shl $0x8,%edx 50: shl $0x8,%edx
53: shr $0x8,%eax 53: shr $0x8,%eax
56: or %eax,%edx 56: or %eax,%edx
58: mov %dx,0xc(%ebx) + 58: test %esi,%esi
5c: mov 0xc(%ebp),%edx + 5a: mov %dx,0xc(%ebx)
5f: test %esi,%esi + 5e: jne 69 <eth_header+0x69>
61: mov 0xb0(%edx),%al + 60: mov 0xc(%ebp),%esi
67: je 71 <eth_header+0x71> + 63: add $0x90,%esi
69: movzbl %al,%eax + 69: mov 0xc(%ebp),%edx
6c: lea 0x6(%ebx),%edi + 6c: movzbl 0xb0(%edx),%eax
6f: jmp 80 <eth_header+0x80> + 73: mov %eax,%ecx
71: mov 0xc(%ebp),%edx + 75: lea 0x6(%ebx),%edi
74: movzbl %al,%eax + 78: shr $0x2,%ecx
77: lea 0x6(%ebx),%edi + 7b: repz movsl %ds:(%esi),%es:(%edi)
7a: lea 0x90(%edx),%esi + 7d: mov %eax,%ecx
80: mov %eax,%ecx + 7f: and $0x3,%ecx
82: shr $0x2,%ecx + 82: je 86 <eth_header+0x86>
85: repz movsl %ds:(%esi),%es:(%edi) + 84: repz movsb %ds:(%esi),%es:(%edi)
87: mov %eax,%ecx + 86: testb $0x88,0x58(%edx)
89: and $0x3,%ecx + 8a: je b1 <eth_header+0xb1>
8c: je 90 <eth_header+0x90> + 8c: movzbl 0xb0(%edx),%esi
8e: repz movsb %ds:(%esi),%es:(%edi) + 93: mov %esi,%ecx
90: mov 0xc(%ebp),%eax + 95: shr $0x2,%ecx
93: testb $0x88,0x58(%eax) + 98: mov %ebx,%edi
97: je bc <eth_header+0xbc> + 9a: xor %eax,%eax
99: movzbl 0xb0(%eax),%edx + 9c: mov %esi,%edx
a0: mov %edx,%ecx + 9e: repz stos %eax,%es:(%edi)
a2: xor %eax,%eax
a4: shr $0x2,%ecx
a7: mov %ebx,%edi
a9: repz stos %eax,%es:(%edi)
..
--- linux-2.6.12-rc2.src/net/ethernet/eth.c.orig Thu Mar 3 09:31:21 2005
+++ linux-2.6.12-rc2.src/net/ethernet/eth.c Wed Jun 22 11:13:54 2005
@@ -92,10 +92,9 @@ int eth_header(struct sk_buff *skb, stru
* Set the source hardware address.
*/
- if(saddr)
- memcpy(eth->h_source,saddr,dev->addr_len);
- else
- memcpy(eth->h_source,dev->dev_addr,dev->addr_len);
+ if(!saddr)
+ saddr = dev->dev_addr;
+ memcpy(eth->h_source,saddr,dev->addr_len);
/*
* Anyway, the loopback-device should never use this function...
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: Eric Dumazet @ 2005-06-22 7:27 UTC (permalink / raw)
To: David S. Miller
Cc: gandalf, hadi, shemminger, mitch.a.williams, john.ronciak, mchan,
buytenh, jdmason, netdev, Robert.Olsson, ganesh.venkatesan,
jesse.brandeburg
In-Reply-To: <20050621.133704.08321534.davem@davemloft.net>
David S. Miller a écrit :
>
> Also, e1000 sends full MTU sized SKBs down into the stack even if the
> packet is very small. This also hurts performance a lot. As
> discussed elsewhere, it should use a "small packet" cut-off just like
> other drivers do. If the RX frame is less than this cut-off value, a
> new smaller sized SKB is allocated and the RX data copied into it.
> The RX ring SKB is left in-place and given back to the chip.
>
> My only guess is that the e1000 driver implemented things this way
> to simplify the RX recycling logic. Well, it is an area ripe for
> improvement in this driver :)
>
>
Here is a copy of a mail from Scott Feldman (19/11/2003) when I asked him to add this copybreak feature into e1000 driver :
It did improve performance on my workload. It also reduce the memory requirement a *lot* (It was using 300.000 active TCP sockets, mostly
receiving short frames)
Eric Dumazet
---------------------------------------------------
Try this (untested) patch. It's against 5.2.26 (which you don't have),
so hand patch it. (Sorry).
Do you have any way to measure performance? CPU utilization? The copy
isn't free.
Oh, also, this patch doesn't try to recycle the 4K skb that was copied
from. Instead, it's freed and re-allocated. Shouldn't be a big deal
because your totally system memory allocation should remain constant
(except for outstanding copybreak skb's).
Let us know how it goes.
-scott
----------------
diff -Naurp e1000-5.2.26/src/e1000_main.c
e1000-5.2.26-cb/src/e1000_main.c
--- e1000-5.2.26/src/e1000_main.c 2003-11-17 19:23:38.000000000
-0800
+++ e1000-5.2.26-cb/src/e1000_main.c 2003-11-18 18:18:07.000000000
-0800
@@ -2343,6 +2343,20 @@ e1000_clean_rx_irq(struct e1000_adapter
}
}
+ /* RONCH 11/18/03 - code added for copybreak test */
+#define E1000_CB_LENGTH 128
+ if(length < E1000_CB_LENGTH ) {
+ struct sk_buff *new_skb = dev_alloc_skb(length
+2);
+ if(new_skb) {
+ skb_reserve(new_skb, 2);
+ new_skb->dev = netdev;
+ memcpy(new_skb->data, skb->data,
length);
+ dev_kfree_skb(skb);
+ skb = new_skb;
+ }
+ }
+ /* end copybreak code */
+
/* Good Receive */
skb_put(skb, length - ETHERNET_FCS_SIZE);
^ permalink raw reply
* Re: [PATCH] tg3_msi() and weakly ordered memory
From: Grant Grundler @ 2005-06-22 5:20 UTC (permalink / raw)
To: David S. Miller; +Cc: iod00d, mchan, netdev
In-Reply-To: <20050621.165634.07642938.davem@davemloft.net>
On Tue, Jun 21, 2005 at 04:56:34PM -0700, David S. Miller wrote:
>
> Ok, here is the patch I came up with as a result of this thread.
looks good to me.
> Michael stated he would investigate using a pure tag comparison in
> place of tg3_has_work() when the chip is using tagged interrupts.
The more I think about it, the more I like the idea of each ISR calling
into a different tg3_poll routine. The specific _poll() routine could
do the "is there more work" checking instead the TX/RX ring
cleanup code. The main reason is the "more work" checks can
be better optimized for MSI (use tags) vs IRQ Line interrupt (use ring
indices) handlers. I also hope to reduce cacheline movement by touching
the status block fewer times.
This isn't a trivial patch and I'm short on time (preparing stuff
for OLS and HP World before my vacation). If there is still interest,
I can prototype a patch in late August or Sept (about 8 weeks from now).
> Thanks.
Welcome and thanks too.
BTW, I greatly appreciate Michael clarifying tg3 behavior.
thanks,
grant
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: Chris Friesen @ 2005-06-22 4:44 UTC (permalink / raw)
To: Donald Becker; +Cc: Andi Kleen, Rick Jones, netdev, davem
In-Reply-To: <Pine.LNX.4.44.0506211642290.21812-100000@bluewest.scyld.com>
Donald Becker wrote:
> On Wed, 22 Jun 2005, Andi Kleen wrote:
>
>
>>>While much has changed since then, the same basic parameters remain
>>> - cache line size
>>
>>In 96 we had 32 byte cache lines. These days 64-128 are common,
>>with some 256 byte cache line systems around.
>
>
> Good point.
> I believe that the most common line size is 64 bytes for L1 cache.
If I recall, G4 chips are 32 bytes, and G5s are 128 bytes. Most current
x86 chips are 64 bytes though.
Chris
^ permalink raw reply
* Re: iptables bug
From: Patrick McHardy @ 2005-06-22 1:52 UTC (permalink / raw)
To: Stephen Jones
Cc: Andrew Morton, netdev, J.A. Magallon,
Netfilter Development Mailinglist, linux-kernel
In-Reply-To: <42B868CC.1010008@hivemynd.net>
Stephen Jones wrote:
>>> "J.A. Magallon" <jamagallon@able.es> wrote:
>>>
>>>> I see strange things.
>>>> When I use bittorrent (azureus or bittorrent-gui), at the same time as
>>>> iptables (for nat and internet access for my ibook), when I stop a
>>>> download
>>>> or exit from one of this apps my external network goes down. I have
>>>> tried the same without iptables loaded and it works fine.
>
> Various linux based firewall forums have posts describing the same
> behavior as the OP of this thread.
>
> Here is one relatively recent example:
>
> http://community.smoothwall.org/forum/viewtopic.php?p=43812#43812
The report seems rather inconclusive, I would appreciate it if the
original poster could narrow down the problem.
Regards
Patrick
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: Donald Becker @ 2005-06-22 0:08 UTC (permalink / raw)
To: Andi Kleen; +Cc: Rick Jones, netdev, davem
In-Reply-To: <20050621223436.GG14251@wotan.suse.de>
On Wed, 22 Jun 2005, Andi Kleen wrote:
> > While much has changed since then, the same basic parameters remain
> > - cache line size
>
> In 96 we had 32 byte cache lines. These days 64-128 are common,
> with some 256 byte cache line systems around.
Good point.
I believe that the most common line size is 64 bytes for L1 cache.
Most L2 caches that have larger line sizes still fill only 64 byte
blocks unless prefetching is triggered. (Feel free to correct me with
non-obscure CPUs and relevant cases. For instance, I know that on the
Itanium the 128 byte line L2 cache is used as L1, but only for FPU
fetches. That doesn't count.)
The implication here is that as soon as we look at the first byte of the
MAC address, we have read in 64 bytes. That's a whole minimum-size
EThernet frame.
> > - frame header size (MAC+IP+ProtocolHeader)
>
> In 96 people tended to not use time stamps.
Ehh, not a big difference.
> > - hot cache lines from copying or type classification
> Not sure what you mean with that.
See the comment above. We decide if a packet is multicast vs. unicast, IP
vs. other at approximately interrupt/"rx_copybreak" time. Very few NIC
provide this info in status bits, so we end up looking at the packet
header. That read moves the previously known-uncached data (after all, it
was just came in from a bus write) into the L1 cache for the CPU handling
the device. Once it's there, the copy is almost free.
[[ Background: Yes, the allocating the new skbuff is very expensive. But
we can either allocate a new, correctly-sized skbuff to copy into, or
allocate a new full-sized skbuff to replace the one we will send to the Rx
queue. ]]
> > - cold memory lines from PCI writes
>
> I suspect in '96 chipsets also didn't do as aggressive prefetching
> as they do today.
Prefetching helps linear read bandwidth, but we shouldn't be triggering
it. And I claim that cache line prefetching only restores the relative
balance between L1/L2 caches, otherwise the long L2 cache lines would be
very expensive with bump-read-bump-read with linear scans through memory.
--
Donald Becker becker@scyld.com
Scyld Software Scyld Beowulf cluster systems
914 Bay Ridge Road, Suite 220 www.scyld.com
Annapolis MD 21403 410-990-9993
^ permalink raw reply
* Re: [PATCH] tg3_msi() and weakly ordered memory
From: David S. Miller @ 2005-06-21 23:56 UTC (permalink / raw)
To: iod00d; +Cc: mchan, netdev
In-Reply-To: <20050614211530.GB25516@esmail.cup.hp.com>
Ok, here is the patch I came up with as a result of this thread.
Michael stated he would investigate using a pure tag comparison in
place of tg3_has_work() when the chip is using tagged interrupts.
Thanks.
[TG3]: Fix missing memory barriers and SD_STATUS_UPDATED bit clearing.
There must be a rmb() between reading the status block tag
and calling tg3_has_work(). This was missing in tg3_mis()
and tg3_interrupt_tagged(). tg3_poll() got it right.
Also, SD_STATUS_UPDATED must be cleared in the status block
right before we call tg3_has_work(). Only tg3_poll() got this
wrong.
Based upon patches and commentary from Grant Grundler and
Michael Chan.
Signed-off-by: David S. Miller <davem@davemloft.net>
--- 1/drivers/net/tg3.c.~1~ 2005-06-21 16:39:19.000000000 -0700
+++ 2/drivers/net/tg3.c 2005-06-21 16:47:55.000000000 -0700
@@ -2929,6 +2929,7 @@
if (tp->tg3_flags & TG3_FLAG_TAGGED_STATUS)
tp->last_tag = sblk->status_tag;
rmb();
+ sblk->status &= ~SD_STATUS_UPDATED;
/* if no more work, tell net stack and NIC we're done */
done = !tg3_has_work(tp);
@@ -2964,6 +2965,7 @@
*/
tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000001);
tp->last_tag = sblk->status_tag;
+ rmb();
sblk->status &= ~SD_STATUS_UPDATED;
if (likely(tg3_has_work(tp)))
netif_rx_schedule(dev); /* schedule NAPI poll */
@@ -3051,6 +3053,7 @@
tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
0x00000001);
tp->last_tag = sblk->status_tag;
+ rmb();
sblk->status &= ~SD_STATUS_UPDATED;
if (likely(tg3_has_work(tp)))
netif_rx_schedule(dev); /* schedule NAPI poll */
^ permalink raw reply
* Re: [patch,rfc] allow registration of multiple netpolls per interface
From: Matt Mackall @ 2005-06-21 22:52 UTC (permalink / raw)
To: Jeff Moyer; +Cc: netdev, netdev, linux-kernel
In-Reply-To: <17080.35214.507402.998984@segfault.boston.redhat.com>
On Tue, Jun 21, 2005 at 05:41:34PM -0400, Jeff Moyer wrote:
> Hi,
>
> This patch restores functionality that was removed when the recursive
> ->poll bug was fixed. Namely, it allows multiple netpoll clients to
> register against the same network interface.
Thanks. I've been neglecting this for a bit while I've been busy with
other things.
> In order to put things into perspective, I'm going to provide some
> background information. So, here is how things used to work:
>
> Multiple users of the netpoll interface could register themselves to send
> packets over the same interface. Any number of these netpoll clients could
> register an rx_hook, as well. However, only the very first in the list
> (hence the last one that registered), that matched the incoming interface,
> would be called when a packet arrived. The reason for this was not design,
> it was an oversight in the implementation. In practice, however, no one
> ever stumbled over this. (There are more subtleties when dealing with
> multiple rx_hooks registered to the same interface, but we'll ignore these,
> since no one ever ran into such problems.)
Hmm. It's conceivable we'll want netdump and kgdb on the same
interface so we'll have to address this eventually..
> Note that each netpoll client that registered an rx_hook was put on a
> netpoll_rx_list. This list was protected by a spinlock, and so operations
> which touched the rx routines would incur a locking penalty and a list
> traversal. I am mentioning this because the list and associated lock were
> removed when the code was refactored, and the patches I propose will
> reintroduce the lock, but not the list.
..so we'll probably want the list back in some form. Sigh.
> Moving to what we have today:
>
> Multiple netpoll clients can register to send packets over the same
> interface. That's right, you can actually do this. However, there are
> ugly side effects. Because we now have a pointer from the net_device to a
> struct netpoll, the last netpoll client to register will be pointed to by
> the net_device->np. What this means is that if you had two clients, the
> first registers an rx_hook and the second does not, then the netpoll code
> will not know that any device has actually registered an rx_hook (since the
> np pointer in the struct net_device is overwritten)! As a result, no
> incoming packets will be delivered to the registered rx routine. This is
> clearly undesirable behaviour.
>
> So what does the patch do?
>
> I created a new structure:
>
> struct netpoll_info {
> spinlock_t poll_lock;
> int poll_owner;
> int rx_flags;
> spinlock_t rx_lock;
> struct netpoll *rx_np; /* netpoll that registered an rx_hook */
> };
>
> This is the structure which gets pointed to by the net_device. All of the
> flags and locks which are specific to the INTERFACE go here. Any variables
> which must be kept per struct netpoll were left in the struct netpoll. So
> now, we have a cleaner separation of data and its scope.
>
> Since we never really supported having more than one struct netpoll
> register an rx_hook, I got rid of the rx_list. This is replaced by a
> single pointer in the netpoll_info structure (np_rx). We still need to
> protect addition or removal of the rx_np pointer, and so keep the lock
> (rx_lock). There is one lock per struct net_device, and I am certain that
> it will be 0 contention, as rx_np will only be changed during an insmod or
> rmmod. If people think this would be a good rcu candidate, let me know and
> I'll change it to use that locking scheme.
It might be simpler to have a single lock here..?
> In the process of making these changes, I've fixed a couple other minor
> bugs [1]. These fixes are included in this patch, but I will break them
> out if people agree with this approach.
>
> I have tested this by registering multiple netpoll clients, and verifying
> that they both function properly. I have not yet tried registering an
> rx_hook, but I believe the code should be sufficient to handle that case.
>
> And so, here is the full patch. I'd appreciate comments. Once we've
> reached consensus, I will resubmit as a patch series.
I think the general idea is sound. So let's take a look at the patch itself.
> Oh, and I've cc'd both netdev@oss.sgi.com and @vger.kernel.org. Is it safe
> to just use the vger list?
Yes.
> [1] netpoll_poll_unlock unlocked and then set the poll_owner. I've
> reversed the order of those operations. The netpoll_cleanup code could
> dereference a null pointer, that was fixed by virtue of being very
> different in the new case.
Ok, let's fix the lock ordering bit first.
> --- linux-2.6.12-rc6/net/core/netpoll.c.orig 2005-06-20 19:51:56.000000000 -0400
> +++ linux-2.6.12-rc6/net/core/netpoll.c 2005-06-21 16:03:22.409620400 -0400
> @@ -131,18 +131,19 @@ static int checksum_udp(struct sk_buff *
> static void poll_napi(struct netpoll *np)
> {
> int budget = 16;
> + struct netpoll_info *npinfo = np->dev->npinfo;
As a minor point of style, I like to put the "get my private info"
lines first.
> @@ -245,6 +246,7 @@ repeat:
> static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
> {
> int status;
> + struct netpoll_info *npinfo;
>
> repeat:
> if(!np || !np->dev || !netif_running(np->dev)) {
> @@ -253,7 +255,8 @@ repeat:
> }
>
> /* avoid recursion */
> - if(np->poll_owner == smp_processor_id() ||
> + npinfo = np->dev->npinfo;
Again, the npinfo assignment ought to happen as soon as possible.
> + if(npinfo->poll_owner == smp_processor_id() ||
> np->dev->xmit_lock_owner == smp_processor_id()) {
> if (np->drop)
> np->drop(skb);
> @@ -346,7 +349,15 @@ static void arp_reply(struct sk_buff *sk
> int size, type = ARPOP_REPLY, ptype = ETH_P_ARP;
> u32 sip, tip;
> struct sk_buff *send_skb;
> - struct netpoll *np = skb->dev->np;
> + struct netpoll *np;
> + struct netpoll_info *npinfo = skb->dev->npinfo;
> +
> + if (!npinfo) return;
We should only be replying to ARPs if we're trapped, right? How do we
get here with npinfo unset?
The return ought to be on a separate line, btw.
> + spin_lock_irqsave(&npinfo->rx_lock, flags);
> + if (npinfo->rx_np->dev == skb->dev)
> + np = npinfo->rx_np;
> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
And I think that means we don't need the lock here either.
> if (!np) return;
And the same question and style criticism of my own code.
> @@ -429,9 +440,9 @@ int __netpoll_rx(struct sk_buff *skb)
> int proto, len, ulen;
> struct iphdr *iph;
> struct udphdr *uh;
> - struct netpoll *np = skb->dev->np;
> + struct netpoll *np = skb->dev->npinfo->rx_np;
>
> - if (!np->rx_hook)
> + if (!np)
> goto out;
> if (skb->dev->type != ARPHRD_ETHER)
> goto out;
> @@ -611,9 +622,8 @@ int netpoll_setup(struct netpoll *np)
> {
> struct net_device *ndev = NULL;
> struct in_device *in_dev;
> -
> - np->poll_lock = SPIN_LOCK_UNLOCKED;
> - np->poll_owner = -1;
> + struct netpoll_info *npinfo;
> + unsigned long flags;
>
> if (np->dev_name)
> ndev = dev_get_by_name(np->dev_name);
> @@ -624,7 +634,17 @@ int netpoll_setup(struct netpoll *np)
> }
>
> np->dev = ndev;
> - ndev->np = np;
> + if (!ndev->npinfo) {
> + npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
> + if (!npinfo)
> + goto release;
> +
> + npinfo->rx_np = NULL;
> + npinfo->poll_lock = SPIN_LOCK_UNLOCKED;
> + npinfo->poll_owner = -1;
> + npinfo->rx_lock = SPIN_LOCK_UNLOCKED;
> + } else
> + npinfo = ndev->npinfo;
>
> if (!ndev->poll_controller) {
> printk(KERN_ERR "%s: %s doesn't support polling, aborting.\n",
> @@ -692,13 +712,20 @@ int netpoll_setup(struct netpoll *np)
> np->name, HIPQUAD(np->local_ip));
> }
>
> - if(np->rx_hook)
> - np->rx_flags = NETPOLL_RX_ENABLED;
> + if(np->rx_hook) {
> + spin_lock_irqsave(&npinfo->rx_lock, flags);
> + npinfo->rx_flags |= NETPOLL_RX_ENABLED;
> + npinfo->rx_np = np;
> + spin_unlock_irqsave(&npinfo->rx_lock, flags);
> + }
> + /* last thing to do is link it to the net device structure */
> + ndev->npinfo = npinfo;
>
> return 0;
>
> release:
> - ndev->np = NULL;
> + if (!ndev->npinfo)
> + kfree(npinfo);
> np->dev = NULL;
> dev_put(ndev);
> return -1;
> @@ -706,9 +733,17 @@ int netpoll_setup(struct netpoll *np)
>
> void netpoll_cleanup(struct netpoll *np)
> {
> - if (np->dev)
> - np->dev->np = NULL;
> - dev_put(np->dev);
> + struct netpoll_info *npinfo;
> +
> + if (np->dev) {
> + npinfo = np->dev->npinfo;
> + if (npinfo && npinfo->rx_np == np) {
> + npinfo->rx_np = NULL;
> + npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
> + }
> + dev_put(np->dev);
> + }
> +
> np->dev = NULL;
> }
>
> --- linux-2.6.12-rc6/net/core/dev.c.orig 2005-06-20 19:51:59.000000000 -0400
> +++ linux-2.6.12-rc6/net/core/dev.c 2005-06-21 13:53:51.583407710 -0400
> @@ -1656,6 +1656,7 @@ int netif_receive_skb(struct sk_buff *sk
> unsigned short type;
>
> /* if we've gotten here through NAPI, check netpoll */
> + /* how else can we get here? --phro */
We can get here in the usual route of non-NAPI delivery, IIRC.
> if (skb->dev->poll && netpoll_rx(skb))
> return NET_RX_DROP;
>
> --- linux-2.6.12-rc6/include/linux/netpoll.h.orig 2005-06-20 19:51:47.000000000 -0400
> +++ linux-2.6.12-rc6/include/linux/netpoll.h 2005-06-21 15:29:48.994422229 -0400
> @@ -16,14 +16,19 @@ struct netpoll;
> struct netpoll {
> struct net_device *dev;
> char dev_name[16], *name;
> - int rx_flags;
> void (*rx_hook)(struct netpoll *, int, char *, int);
> void (*drop)(struct sk_buff *skb);
> u32 local_ip, remote_ip;
> u16 local_port, remote_port;
> unsigned char local_mac[6], remote_mac[6];
> +};
> +
> +struct netpoll_info {
> spinlock_t poll_lock;
> int poll_owner;
> + int rx_flags;
> + spinlock_t rx_lock;
> + struct netpoll *rx_np; /* netpoll that registered an rx_hook */
> };
>
> void netpoll_poll(struct netpoll *np);
> @@ -39,22 +44,35 @@ void netpoll_queue(struct sk_buff *skb);
> #ifdef CONFIG_NETPOLL
> static inline int netpoll_rx(struct sk_buff *skb)
> {
> - return skb->dev->np && skb->dev->np->rx_flags && __netpoll_rx(skb);
> + struct netpoll_info *npinfo = skb->dev->npinfo;
> + unsigned long flags;
> + int ret = 0;
> +
> + if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
> + return 0;
> +
> + spin_lock_irqsave(&npinfo->rx_lock, flags);
> + /* check rx_flags again with the lock held */
> + if (npinfo->rx_flags && __netpoll_rx(skb))
> + ret = 1;
> + spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> +
> + return ret;
> }
This is perhaps a problem due to cache line bouncing. Perhaps we can
use an atomic op and a memory barrier instead?
> static inline void netpoll_poll_lock(struct net_device *dev)
> {
> - if (dev->np) {
> - spin_lock(&dev->np->poll_lock);
> - dev->np->poll_owner = smp_processor_id();
> + if (dev->npinfo) {
> + spin_lock(&dev->npinfo->poll_lock);
> + dev->npinfo->poll_owner = smp_processor_id();
> }
> }
>
> static inline void netpoll_poll_unlock(struct net_device *dev)
> {
> - if (dev->np) {
> - spin_unlock(&dev->np->poll_lock);
> - dev->np->poll_owner = -1;
> + if (dev->npinfo) {
> + dev->npinfo->poll_owner = -1;
> + spin_unlock(&dev->npinfo->poll_lock);
> }
> }
>
> --- linux-2.6.12-rc6/include/linux/netdevice.h.orig 2005-06-20 20:26:21.000000000 -0400
> +++ linux-2.6.12-rc6/include/linux/netdevice.h 2005-06-21 14:46:52.093190854 -0400
> @@ -41,7 +41,7 @@
> struct divert_blk;
> struct vlan_group;
> struct ethtool_ops;
> -struct netpoll;
> +struct netpoll_info;
> /* source back-compat hooks */
> #define SET_ETHTOOL_OPS(netdev,ops) \
> ( (netdev)->ethtool_ops = (ops) )
> @@ -468,7 +468,7 @@ struct net_device
> unsigned char *haddr);
> int (*neigh_setup)(struct net_device *dev, struct neigh_parms *);
> #ifdef CONFIG_NETPOLL
> - struct netpoll *np;
> + struct netpoll_info *npinfo;
> #endif
> #ifdef CONFIG_NET_POLL_CONTROLLER
> void (*poll_controller)(struct net_device *dev);
--
Mathematics is the supreme nostalgia of our time.
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-21 22:34 UTC (permalink / raw)
To: Donald Becker; +Cc: Andi Kleen, Rick Jones, netdev, davem
In-Reply-To: <Pine.LNX.4.44.0506211507110.21812-100000@bluewest.scyld.com>
> While much has changed since then, the same basic parameters remain
> - cache line size
In 96 we had 32 byte cache lines. These days 64-128 are common,
with some 256 byte cache line systems around.
> - frame header size (MAC+IP+ProtocolHeader)
In 96 people tended to not use time stamps.
> - hot cache lines from copying or type classification
Not sure what you mean with that.
> - cold memory lines from PCI writes
I suspect in '96 chipsets also didn't do as aggressive prefetching
as they do today.
-Andi
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: Donald Becker @ 2005-06-21 22:22 UTC (permalink / raw)
To: Andi Kleen; +Cc: Rick Jones, netdev, davem
In-Reply-To: <p73oe9zbcx0.fsf@verdi.suse.de>
On 21 Jun 2005, Andi Kleen wrote:
> Rick Jones <rick.jones2@hp.com> writes:
>
> > > Actually, it has a _HUGE_ _HUGE_ impact. If you pass the big buffer
> > > up, the receiving socket gets charged for the size of the huge buffer,
> > > not for just the size of the packet contained within. This makes
> > > sockets get overcharged for data reception, and it can cause all kinds
> > > of performance problems.
> >
> > Then copy when the socket is about to fill with overhead bytes?
Or better, predict when the frame you are currently stuffing into the
queue will be there when the queue fills up. And then use the same
crystal ball to...
> Mostly because it is the only sane way to handle devices with very big
> MTU. But it turns off all kinds of fast paths before it happens, I
> guess that is what David was refering too.
>
> However I suspect the cut-off points with rx-copybreak in common driver
> have been often tuned before that code was introduced and it might
> be worth to do some retesting.
Most of that analysis and tuning was done in the 1996-99 timeframe.
While much has changed since then, the same basic parameters remain
- cache line size
- frame header size (MAC+IP+ProtocolHeader)
- hot cache lines from copying or type classification
- cold memory lines from PCI writes
I suspect you'll find that a good rx_copybreak is pretty much the same as
it was when I did the original evaluation.
If you are looking for an area that has changed: the hidden cost of
maintaining consistent cache lines on SMP systems is far higher than it
was back in the days of the Pentium Pro.
Donald Becker becker@scyld.com
Scyld Software A Penguin Computing company
914 Bay Ridge Road, Suite 220 www.scyld.com
Annapolis MD 21403 410-990-9993
^ permalink raw reply
* Re: [patch] devinet: cleanup if statements
From: Andi Kleen @ 2005-06-21 21:51 UTC (permalink / raw)
To: David S. Miller; +Cc: pmeda, netdev
In-Reply-To: <20050621.134822.21926602.davem@davemloft.net>
"David S. Miller" <davem@davemloft.net> writes:
>
> This whole area is pretty messy, we should examine the true
> intended semantics of the ifa_label stuff.
Perhaps it would be best to just retire that 2.0 alias compatibility
cruft now. Everybody should be using ifconfig or iproute2 by now
that can add or remove addresses directly without the compatibility
syntax.
They never worked fully in corner cases anyways, e.g. SIOCSIFNAME with
these aliases was always a problem.
-Andi
^ permalink raw reply
* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-21 21:47 UTC (permalink / raw)
To: Rick Jones; +Cc: netdev, davem
In-Reply-To: <42B87ACF.3080800@hp.com>
Rick Jones <rick.jones2@hp.com> writes:
> > Actually, it has a _HUGE_ _HUGE_ impact. If you pass the big buffer
> > up, the receiving socket gets charged for the size of the huge buffer,
> > not for just the size of the packet contained within. This makes
> > sockets get overcharged for data reception, and it can cause all kinds
> > of performance problems.
>
> Then copy when the socket is about to fill with overhead bytes?
The stack has supported that since 2.4.
Mostly because it is the only sane way to handle devices with very big
MTU. But it turns off all kinds of fast paths before it happens, I
guess that is what David was refering too.
However I suspect the cut-off points with rx-copybreak in common driver
have been often tuned before that code was introduced and it might
be worth to do some retesting.
-Andi
^ 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