Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH 0/10] [IOAT] I/OAT patches repost
From: Chris Leech @ 2006-04-27 23:45 UTC (permalink / raw)
  To: David S. Miller; +Cc: olof, andrew.grover, netdev
In-Reply-To: <20060420.204200.103377406.davem@davemloft.net>

On 4/20/06, David S. Miller <davem@davemloft.net> wrote:
> Yes, and it means that the memory bandwidth costs are equivalent
> between I/O AT and cpu copy.

The following is a response from the I/OAT architects.  I only point
out that this is not coming directly from me because I have not seen
the data to verify the claims regarding the speed of a copy vs a load
and the cost of the rep mov instruction.  I'll encourage more direct
participation in this discussion from the architects moving forward.

    - Chris

Let's talk about the caching benefits that is seemingly lost when
using the DMA engine. The intent of the DMA engine is to save CPU
cycles spent in copying data (rep mov). In cases where the destination
is already warm in cache (due to destination buffer re-use) and the
source is in memory, the cycles spent in a host copy is not just due
to the cache misses it encounters in the process of bringing in the
source but also due to the execution of rep move itself within the
host core. If you contrast this to simply touching (loading) the data
residing in memory, the cost of this load is primarily the cost of the
cache misses and not so much CPU execution time. Given this, some of
the following points are noteworthy:

1. While the DMA engine forces the destination to be in memory and
touching it may cause the same number of observable cache misses as a
host copy assuming a cache warmed destination, the cost of the host
copy (in terms of CPU cycles) is much more than the cost of the touch.

2. CPU hardware prefetchers do a pretty good job of staying ahead of
the fetch stream to minimize cache misses. So for loads of medium to
large buffers, cache misses form a much smaller component of the data
fetch time…most of it is dominated by front side bus (FSB) or Memory
bandwidth. For small buffers, we do not use the DMA engine but if we
had to, we would insert SW prefetches that do reasonably well.

3. If the destination wasn't already warm in cache i.e., it was in
memory or some CPU other cache, host copy will have to snoop and bring
the destination in and will encounter additional misses on the
destination buffer as well. These misses are the same as those
encountered in #1 above when using the DMA engine and touching the
data afterwards. So in effect it becomes a wash when compared to the
DMA engine's behavior. The case where the destination is already warm
in cache is common in benchmarks such as iperf, ttcp etc. where the
same buffer is reused over and over again. Real applications typically
will not exhibit this aggressive buffer re-use behavior.

4. It may take a large number of packets (and several interrupts) to
satisfy a large posted buffer (say 64KB). Even if you use host copy to
warm the cache with the destination, there is no guarantee that some
or all of the destination will stay in the cache before the
application has a chance to read the data.

5. The source data payload (skb ->data) is typically needed only once
for the copy and has no use later. The host copy brings it into the
cache and may end up polluting the cache, and consuming FSB bandwidth
whereas the DMA engine avoids this altogether.

The IxChariot data posted earlier that touches the data and yet shows
I/OAT benefit is due to some of the reasons above. Bottom line is that
I agree with the cache benefit argument of host copy for small buffers
(64B to 512B) but for larger buffers and certain application scenarios
(destination in memory), the DMA engine will show better performance
regardless of where the destination buffer resided to begin with and
where it is accessed from.

^ permalink raw reply

* Re: [PATCH 0/10] [IOAT] I/OAT patches repost
From: Chris Leech @ 2006-04-27 23:49 UTC (permalink / raw)
  To: Rick Jones; +Cc: David S. Miller, olof, andrew.grover, netdev
In-Reply-To: <4449127A.8050404@hp.com>

> Netperf2 TOT now accesses the buffer that was just recv()'d rather than
> the one that is about to be recv()'d.

We've posted netperf2 results with I/OAT enabled/disabled and the data
access option on/off at
http://kernel.org/pub/linux/kernel/people/grover/ioat/netperf-icb-1.5-postscaling-both.pdf

This link has also been added to the I/OAT page on the LinuxNet wiki.

- Chris

^ permalink raw reply

* Re: [PATCH 0/10] [IOAT] I/OAT patches repost
From: Rick Jones @ 2006-04-27 23:53 UTC (permalink / raw)
  To: chris.leech; +Cc: David S. Miller, olof, andrew.grover, netdev
In-Reply-To: <41b516cb0604271649o31315086ma1c82b824da263e7@mail.gmail.com>

Chris Leech wrote:
>>Netperf2 TOT now accesses the buffer that was just recv()'d rather than
>>the one that is about to be recv()'d.
> 
> 
> We've posted netperf2 results with I/OAT enabled/disabled and the data
> access option on/off at
> http://kernel.org/pub/linux/kernel/people/grover/ioat/netperf-icb-1.5-postscaling-both.pdf
> 

calling it netperf data verification is a quite overstated - all netperf 
does is read from or write to the buffer.  there is no check of data 
integrity or anything

rick jones

^ permalink raw reply

* E1000 stopped transmitting in rc3.
From: Dave Jones @ 2006-04-28  0:13 UTC (permalink / raw)
  To: netdev

With 2.6.17-rc3, my E1000 won't get a dhcp lease.
Looking at tcpdump and ifconfig output, it's easy to see why.
It's recieving packets, but the packets transmitted field
of ifconfig never increases.

The last version I have built that worked ok was 2.6.17rc2-git3

NIC is ..
03:0e.0 Ethernet controller: Intel Corporation 82545GM Gigabit Ethernet Controller (rev 04)
(8086:1026)

		Dave

-- 
http://www.codemonkey.org.uk

^ permalink raw reply

* [patch 20/24] NET: e1000: Update truesize with the length of the packet for packet split
From: Greg KH @ 2006-04-28  0:22 UTC (permalink / raw)
  To: linux-kernel, stable, jgarzik
  Cc: Justin Forbes, Zwane Mwaikambo, Theodore Ts'o, Randy Dunlap,
	Dave Jones, Chuck Wolber, torvalds, akpm, alan, netdev,
	jesse.brandeburg, john.ronciak, Jeffrey.t.kirsher, auke, davem,
	Auke Kok, Greg Kroah-Hartman
In-Reply-To: <20060428001557.GA18750@kroah.com>

[-- Attachment #1: net-e1000-update-truesize-with-the-length-of-the-packet-for-packet-split.patch --]
[-- Type: text/plain, Size: 764 bytes --]

-stable review patch.  If anyone has any objections, please let us know.

------------------

Update skb with the real packet size.


Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
Signed-off-by: John Ronciak <john.ronciak@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/net/e1000/e1000_main.c |    1 +
 1 file changed, 1 insertion(+)

--- linux-2.6.16.11.orig/drivers/net/e1000/e1000_main.c
+++ linux-2.6.16.11/drivers/net/e1000/e1000_main.c
@@ -3851,6 +3851,7 @@ e1000_clean_rx_irq_ps(struct e1000_adapt
 			skb_shinfo(skb)->nr_frags++;
 			skb->len += length;
 			skb->data_len += length;
+			skb->truesize += length;
 		}
 
 		e1000_rx_checksum(adapter, staterr,

--

^ permalink raw reply

* Re: [LARTC] how to change classful netem loss probability?
From: George P Nychis @ 2006-04-28  1:18 UTC (permalink / raw)
  To: George Nychis; +Cc: lartc, netdev
In-Reply-To: <444FF846.7050600@cmu.edu>

And if its not possible to change the probability, is there another method I can use instead?

> Hi,
> 
> I am using netem to add loss and then adding another qdisc within netem 
> according to the wiki.  Then i want to change the netem drop probability 
> without having to delete the qdisc and recreate it.  I try it but I get 
> invalid argument:
> 
> thorium-ini hedpe # tc qdisc add dev ath0 root handle 1:0 netem drop 1% 
> thorium-ini hedpe # tc qdisc add dev ath0 parent 1:1 handle 10: xcp 
> capacity 54Mbit limit 500 thorium-ini hedpe # tc -s qdisc ls dev ath0 qdisc
> netem 1: limit 1000 loss 1% Sent 0 bytes 0 pkts (dropped 0, overlimits 0) 
> qdisc xcp 10: parent 1:1 capacity 52734Kbit limit 500p Sent 0 bytes 0 pkts
> (dropped 0, overlimits 0) thorium-ini hedpe # tc qdisc change dev ath0
> root handle 1:0 netem drop 1% RTNETLINK answers: Invalid argument 
> thorium-ini hedpe # tc qdisc change dev ath0 root netem drop 1% RTNETLINK
> answers: Invalid argument
> 
> any ideas?
> 
> Thanks! George _______________________________________________ LARTC mailing
> list LARTC@mailman.ds9a.nl 
> http://mailman.ds9a.nl/cgi-bin/mailman/listinfo/lartc
> 
> 


-- 


^ permalink raw reply

* Re: E1000 stopped transmitting in rc3.
From: Auke Kok @ 2006-04-28  1:54 UTC (permalink / raw)
  To: Dave Jones; +Cc: netdev
In-Reply-To: <20060428001352.GA3319@redhat.com>

Dave Jones wrote:
> With 2.6.17-rc3, my E1000 won't get a dhcp lease.
> Looking at tcpdump and ifconfig output, it's easy to see why.
> It's recieving packets, but the packets transmitted field
> of ifconfig never increases.
> 
> The last version I have built that worked ok was 2.6.17rc2-git3

*puzzled*

the only patch between 2.6.17rc2 and 2.6.17rc3 contains an rx-path patch, but 
nothing that affects tx. All the other patches sent earlier are queued for 
2.6.18 so they don't apply.

Auke

^ permalink raw reply

* Re: [PATCH 1/3] Rough VJ Channel Implementation - vj_core.patch
From: Evgeniy Polyakov @ 2006-04-28  6:05 UTC (permalink / raw)
  To: David S. Miller; +Cc: kelly, rusty, netdev
In-Reply-To: <20060427.130918.65400512.davem@davemloft.net>

On Thu, Apr 27, 2006 at 01:09:18PM -0700, David S. Miller (davem@davemloft.net) wrote:
> Evgeniy, the difference between this and your work is that you did not
> have an intelligent piece of hardware that could be told to recognize
> flows, and only put packets for a specific flow into that's flow's
> buffer pool.

There are the most "intellegent" NICs which use MMIO copy like Realtek 8139 :)
which were used in receiving zero-copy [1] project.

There was special alorithm researched for receiving zero-copy [1] to allow 
to put not page-aligned TCP frames into pages, but there was other
problem when page was committed, since no byte commit is allowed in VFS.

In this case we do not have that problem, but instead we must force userspace to
be very smart when dealing with mapped buffers, instead of simple recv().
And for sending it must be even smarter, since data must be properly
aligned. And what about crappy hardware which can DMA only into limited
memory area, or NIC that can not do sg? Or do we need remapping for NIC
that can not do checksum calculation?

> > If we want to dma data from nic into premapped userspace area, this will
> > strike with message sizes/misalignment/slow read and so on, so
> > preallocation has even more problems.
> 
> I do not really think this is an issue, we put the full packet into
> user space and teach it where the offset is to the actual data.
> We'll do the same things we do today to try and get the data area
> aligned.  User can do whatever is logical and relevant on his end
> to deal with strange cases.
> 
> In fact we can specify that card has to take some care to get data
> area of packet aligned on say an 8 byte boundary or something like
> that.  When we don't have hardware assist, we are going to be doing
> copies.

Userspace must be too smart, and as we saw with various java tests, it
can not be so even now.
And what if pages are shared and several threads are trying to write
into the same remapped area? Will we use COW and be blamed like Mach
and FreeBSD developers? :)

> > I do think that significant win in VJ's tests belongs not to remapping
> > and cache-oriented changes, but to move all protocol processing into
> > process' context.
> 
> I partly disagree.  The biggest win is eliminating all of the control
> overhead (all of "softint RX + protocol demux + IP route lookup +
> socket lookup" is turned into single flow demux), and the SMP safe
> data structure which makes it realistic enough to always move the bulk
> of the packet work to the socket's home cpu.
> 
> I do not think userspace protocol implementation buys enough to
> justify it.  We have to do the protection switch in and out of kernel
> space anyways, so why not still do the protected protocol processing
> work in the kernel?  It is still being done on the user's behalf,
> contributes to his time slice, and avoids all of the terrible issues
> of userspace protocol implementations.

After hard irq softirq is scheduled, then later userspace is scheduled,
at least 2 context switch just to move a packet, and "slow" userspace
code is interrupted by both irqs again...
I run some tests on ppc32 embedded boards which showed that rescheduling
latency tend to have milliseconds delay sometimes (about 4 running processes
on 200mhz cpu), although we do not have some real-time requirements here
it is not a good sign...

> And I also want to note that even if the whole idea explodes and
> cannot be made to work, there are good arguments for transitioning
> to SKB'less drivers for their own sake.  So work will really not
> be lost.
> 
> Let's have 100 different implementations of net channels! :-)

:)

-- 
	Evgeniy Polyakov

^ permalink raw reply

* Re: [PATCH 1/3] Rough VJ Channel Implementation - vj_core.patch
From: Evgeniy Polyakov @ 2006-04-28  6:10 UTC (permalink / raw)
  To: Caitlin Bestler; +Cc: David S. Miller, kelly, rusty, netdev
In-Reply-To: <54AD0F12E08D1541B826BE97C98F99F143B020@NT-SJCA-0751.brcm.ad.broadcom.com>

On Thu, Apr 27, 2006 at 02:12:09PM -0700, Caitlin Bestler (caitlinb@broadcom.com) wrote:
> So the real issue is when there is an intelligent device that
> uses hardware packet classification to place the packet in
> the correct ring. We don't want to bypass packet filtering,
> but it would be terribly wasteful to reclassify the packet.
> Intelligent NICs will have packet classification capabilities
> to support RDMA and iSCSI. Those capabilities should be available
> to benefit SOCK_STREAM and SOCK_DGRAM users as well without it
> being a choice of either turning all stack control over to
> the NIC or ignorign all NIC capabilities beyound pretending
> to be a dumb Ethernet NIC.

Btw, how is it supposed to work without header split capabale hardware?

-- 
	Evgeniy Polyakov

^ permalink raw reply

* Re: E1000 stopped transmitting in rc3.
From: David S. Miller @ 2006-04-28  7:14 UTC (permalink / raw)
  To: sofar; +Cc: davej, netdev
In-Reply-To: <445175D4.8010209@foo-projects.org>

From: Auke Kok <sofar@foo-projects.org>
Date: Thu, 27 Apr 2006 18:54:28 -0700

> Dave Jones wrote:
> > With 2.6.17-rc3, my E1000 won't get a dhcp lease.
> > Looking at tcpdump and ifconfig output, it's easy to see why.
> > It's recieving packets, but the packets transmitted field
> > of ifconfig never increases.
> > 
> > The last version I have built that worked ok was 2.6.17rc2-git3
> 
> *puzzled*
> 
> the only patch between 2.6.17rc2 and 2.6.17rc3 contains an rx-path patch, but 
> nothing that affects tx. All the other patches sent earlier are queued for 
> 2.6.18 so they don't apply.

If the rx path is corrupting the packets or providing bad
checksums, there will be nothing to transmit back.

So RX changes could explain the behavior.

^ permalink raw reply

* Re: [Fireflier-devel] Re: [PATCH][RFC] Security marking
From: Patrick McHardy @ 2006-04-28  7:19 UTC (permalink / raw)
  To: Török Edwin; +Cc: netdev, James Morris, fireflier-devel
In-Reply-To: <200604232157.59758.edwin@gurde.com>

Török Edwin wrote:
> Patrick what is the status of solving the skfilter issues? Can I help with 
> testing patches,  etc.?

Not yet. If nothing gets in between I plan to get the patches ready
next week.

> On Monday 20 February 2006 18:42, Patrick McHardy wrote:
> 
>>Confirmation of conntrack entries. They shouldn't be confirmed before
>>packets have passed the socket hooks. This is the tricky part because
>>we don't know if packets will be delivered to a raw socket or not
>>when calling the regular LOCAL_IN hook.
>>The only way to solve this 
>>seems to be to use the socket hooks for all incoming packets, that
>>way we can defer confirmation unconditionally.
> 
> Are there any problems with using socket hooks for all packets?

Not really, just that some protocols don't use sockets, so its a bit
pointless for them. OTOH it should make rule management easier if
everything can be done in the same table.

>>The nicest way would 
>>be to just move the regular LOCAL_IN hook to the socket hooks, but
>>this doesn't work with SNAT in LOCAL_IN because the socket lookup
>>needs the already NATed address.
> 
> Move just the non SNAT part of LOCAL_IN to socket hooks?(does this make 
> sense?)

That would be my prefered way, but it changes user-visible behaviour.
Currently filtering is done before SNAT, this change would reverse
that.

^ permalink raw reply

* Re: [PATCH 1/3] Rough VJ Channel Implementation - vj_core.patch
From: David S. Miller @ 2006-04-28  7:20 UTC (permalink / raw)
  To: johnpol; +Cc: caitlinb, kelly, rusty, netdev
In-Reply-To: <20060428061054.GD17360@2ka.mipt.ru>

From: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
Date: Fri, 28 Apr 2006 10:10:54 +0400

> On Thu, Apr 27, 2006 at 02:12:09PM -0700, Caitlin Bestler (caitlinb@broadcom.com) wrote:
> > So the real issue is when there is an intelligent device that
> > uses hardware packet classification to place the packet in
> > the correct ring. We don't want to bypass packet filtering,
> > but it would be terribly wasteful to reclassify the packet.
> > Intelligent NICs will have packet classification capabilities
> > to support RDMA and iSCSI. Those capabilities should be available
> > to benefit SOCK_STREAM and SOCK_DGRAM users as well without it
> > being a choice of either turning all stack control over to
> > the NIC or ignorign all NIC capabilities beyound pretending
> > to be a dumb Ethernet NIC.
> 
> Btw, how is it supposed to work without header split capabale hardware?

I do not see header splitting as a requirement, let the raw
headers sit in the user queue and provide an offset to the data.
All of this page alignment stuff is unnecessary complexity.

I know you think applications are too dumb to be expected to handle
these kinds of things, but how many apps do you expect to convert over
to these new interfaces?

The ones that matter will, and great care will be made by the
programmer who does this.

^ permalink raw reply

* Re: [PATCH 1/3] Rough VJ Channel Implementation - vj_core.patch
From: Evgeniy Polyakov @ 2006-04-28  7:32 UTC (permalink / raw)
  To: David S. Miller; +Cc: caitlinb, kelly, rusty, netdev
In-Reply-To: <20060428.002027.51369887.davem@davemloft.net>

On Fri, Apr 28, 2006 at 12:20:27AM -0700, David S. Miller (davem@davemloft.net) wrote:
> > Btw, how is it supposed to work without header split capabale hardware?
> 
> I do not see header splitting as a requirement, let the raw
> headers sit in the user queue and provide an offset to the data.
> All of this page alignment stuff is unnecessary complexity.
> 
> I know you think applications are too dumb to be expected to handle
> these kinds of things, but how many apps do you expect to convert over
> to these new interfaces?
> 
> The ones that matter will, and great care will be made by the
> programmer who does this.

Ugh, so it will not ring buffer of data _flow_, but ring buffer of
(header+data) packets.

Definitely, userspace application must be very smart to deal with 
ip/tcp/option headers...

-- 
	Evgeniy Polyakov

^ permalink raw reply

* Re: IP1000 gigabit nic driver
From: David Gómez @ 2006-04-28  7:54 UTC (permalink / raw)
  To: Francois Romieu; +Cc: Linux-kernel, netdev
In-Reply-To: <20060427185627.GA30871@electric-eye.fr.zoreil.com>

Hi Francois,

On Apr 27 at 08:56:27, Francois Romieu wrote:
> /me goes to http://www.icplus.com.tw/driver-pp-IP1000A.html
> 
> $ unzip -c IP1000A-Linux-driver-v2.09f.zip | grep MODULE_LICENSE
>     MODULE_LICENSE("GPL");

Thanks, i didn't notice it ;-)

> It's a bit bloaty but it does not seem too bad (not mergeable "as
> is" though). Do you volunteer to test random cra^W^W carefully
> engineered code on your computer to help the rework/merging process ?

Yes, i am. Send me all the the cra\x17\x17\x17 ;-), i'm ready to
test it.

cheers,

-- 
David Gómez                                      Jabber ID: davidge@jabber.org

^ permalink raw reply

* Re: IP1000 gigabit nic driver
From: David Gómez @ 2006-04-28  7:57 UTC (permalink / raw)
  To: David Vrabel; +Cc: Francois Romieu, Linux-kernel, netdev
In-Reply-To: <445144FF.4070703@cantab.net>

Hi David,

On Apr 27 at 11:26:07, David Vrabel wrote:
> I finally got around to putting a 2nd NIC in my box that has one of this 
> chips and was going to start fixing the driver up and preparing it for 
> submission this weekend.

Great!

>  Or I might try rewriting from scratch based on 
> the datasheet depending on how horrific the code looks on closer inspection.

For what i've seen, the code seems quite readable.

> Not got a whole lot of time to do this so no timescale for completion...

I could help. What things do you think need to be fixed before
submitting the driver?

cheers,

-- 
David Gómez                                      Jabber ID: davidge@jabber.org

^ permalink raw reply

* e1000 TX hang problems
From: Gerhard de Jeger @ 2006-04-28  8:13 UTC (permalink / raw)
  To: netdev

I'm also experiencing TX hang problems with the e1000 driver.

I was using the 6.0.60-k2 version previously which shipped with the
2.6.14.2 kernel.
 
I had no problems with this driver, but after my issues with the 7.0.33
driver I tried to go back to the 6.0.60-k driver. Now when I load the
module, there is an error saying that the EEPROM is corrupt - so I
cannot use the old driver which used to work.
 
Why is it that I now get the EEPROM error with the older driver?

Any ideas on how to fix the problem (with either the old driver or the
new one)?

Thanx,
Gerhard


^ permalink raw reply

* RE: [PATCH 1/3] Rough VJ Channel Implementation - vj_core.patch
From: Rusty Russell @ 2006-04-28  8:24 UTC (permalink / raw)
  To: Caitlin Bestler; +Cc: David S. Miller, johnpol, kelly, netdev
In-Reply-To: <54AD0F12E08D1541B826BE97C98F99F143B020@NT-SJCA-0751.brcm.ad.broadcom.com>

On Thu, 2006-04-27 at 14:12 -0700, Caitlin Bestler wrote:
> So the real issue is when there is an intelligent device that
> uses hardware packet classification to place the packet in
> the correct ring. We don't want to bypass packet filtering,
> but it would be terribly wasteful to reclassify the packet.
> Intelligent NICs will have packet classification capabilities
> to support RDMA and iSCSI. Those capabilities should be available
> to benefit SOCK_STREAM and SOCK_DGRAM users as well without it
> being a choice of either turning all stack control over to
> the NIC or ignorign all NIC capabilities beyound pretending
> to be a dumb Ethernet NIC.
> 
> For example, counting packets within an approved connection
> is a valid goal that the final solution should support. But
> would a simple count be sufficient, or do we truly need the
> full flexibility currently found in netfilter?

Note that the problem space AFAICT includes strange advanced routing
setups, ingress qos and possibly others, not just netfilter.  But
perhaps the same solutions apply, so I'll concentrate on nf.

If we start with a "disable direct netchannels when netfilter hooks
registered", we would inevitably refine it to "disable some netchannels
when netfilter hooks registered".  The worst case for this filtering
based on connection tracking, with its constantly changing effects as
things time out.  Hard problem.

Is it time to re-examine the Grand Unified Lookup which Dave mentions
every few years? 8)

> My assumption
> is that each input ring has a matching output ring, and that
> the output ring cannot be used to send packets that would
> not be matched by the reverse rule for the paired input ring.
> So the information that supports enforcing that rule needs
> to be stored somewhere other than the ring itself.

Ah, this is a different problem.  Our idea was to have a syscall which
would check & sanitize the buffers for output.  To do this, you need the
ability to chain buffers (a simple next entry in the header, for us).

Sanitization would copy the header into a global buffer (ie. not one
reachable by userspace), check the flowid, and chain on the rest of the
user buffer.  After it had sanitized the buffers, it would activate the
NIC, which would only send out buffers which started with a kernel
buffer.

Of course, the first step (CAP_NET_RAW-only) wouldn't need this.  And,
if the "sanitize_and_send" syscall were PF_VJCHAN's write(), then the
contents of the write() could actually be the header: userspace would
never deal with chained buffers.

Finally, it's not clear how one should sanely mix this with sendfile
etc.  Maybe you don't, and only use this for RDMA, etc.

Cheers!
Rusty.
-- 
 ccontrol: http://ozlabs.org/~rusty/ccontrol


^ permalink raw reply

* Re: [PATCH 22/32] rt2x00: Allocate ring structures in single array
From: Jiri Benc @ 2006-04-28 10:52 UTC (permalink / raw)
  To: Ivo van Doorn; +Cc: netdev, rt2x00-devel
In-Reply-To: <200604280003.14287.IvDoorn@gmail.com>

On Fri, 28 Apr 2006 00:03:13 +0200, Ivo van Doorn wrote:
> Use seperate function to convert a dscape ring ID
> to the address of the actual ring.

Just minor things:

> [...]
> @@ -1386,20 +1412,19 @@ rt2400pci_tx(struct net_device *net_dev,
>  
>  	rt2x00_register_read(rt2x00pci, TXCSR0, &reg);
>  
> -	if (control->queue == IEEE80211_TX_QUEUE_DATA0) {
> -		ring = &rt2x00pci->prio;
> -		rt2x00_set_field32(&reg, TXCSR0_KICK_PRIO, 1);
> -	} else if (control->queue == IEEE80211_TX_QUEUE_DATA1) {
> -		ring = &rt2x00pci->tx;
> -		rt2x00_set_field32(&reg, TXCSR0_KICK_TX, 1);
> -	} else if (control->queue == IEEE80211_TX_QUEUE_AFTER_BEACON) {
> -		ring = &rt2x00pci->atim;
> -		rt2x00_set_field32(&reg, TXCSR0_KICK_ATIM, 1);
> -	} else {
> -		ERROR("Frame received for invalid queue.");
> +	/*
> +	 * Determine which ring to put packet on.
> +	 */
> +	ring = rt2x00pci_get_ring(rt2x00pci, control->queue);
> +	if (unlikely(!ring)) {

Should not happen. Maybe some message that user should report a bug?

> +		ERROR("Attempt to send packet over invalid queue %d.\n",
> +			control->queue);
>  		return NET_RX_DROP;
>  	}
>  
> +	if (rt2x00_ring_full(ring))
> +		return NET_RX_DROP;

NET_XMIT_DROP?

> +
>  	entry = rt2x00_get_data_entry(ring);
>  	txd = entry->desc_addr;
>  
> [...]
> @@ -1780,14 +1813,17 @@ rt2400pci_conf_tx(struct net_device *net
>  	int queue, const struct ieee80211_tx_queue_params *params)
>  {
>  	struct rt2x00_pci	*rt2x00pci = ieee80211_dev_hw_data(net_dev);
> -	struct data_ring	*ring;
> +	struct data_ring	*ring = &rt2x00pci->ring[RING_TX];
>  
> -	if (queue == IEEE80211_TX_QUEUE_DATA0)
> -		ring = &rt2x00pci->prio;
> -	else if (queue == IEEE80211_TX_QUEUE_DATA1)
> -		ring = &rt2x00pci->tx;
> -	else
> -		return -EINVAL;
> +	/*
> +	 * We don't support variating cw_min and cw_max variables
> +	 * per queue. So by default we only configure the TX queue,
> +	 * and ignore all other configurations.
> +	 */
> +	if (queue != IEEE80211_TX_QUEUE_DATA0) {
> +		NOTICE("Ignoring configuration for queue %d.\n", queue);
> +		return 0;

Is there a reason for not returning a proper error code?

> +	}
>  
>  	memcpy(&ring->tx_params, params, sizeof(*params));
>  

Ditto for rt2500pci and rt2500usb.

Thanks,

-- 
Jiri Benc
SUSE Labs

^ permalink raw reply

* Re: IP1000 gigabit nic driver
From: Pekka Enberg @ 2006-04-28 10:58 UTC (permalink / raw)
  To: David Vrabel, Francois Romieu, Linux-kernel, netdev
In-Reply-To: <20060428075725.GA18957@fargo>

On 4/28/06, David Gómez <david@pleyades.net> wrote:
> I could help. What things do you think need to be fixed before
> submitting the driver?

Needs some serious coding style cleanup and conversion to proper 2.6
APIs for starters.

^ permalink raw reply

* Re: [PATCH 29/32] rt2x00: dscape compatibilitiy
From: Jiri Benc @ 2006-04-28 11:12 UTC (permalink / raw)
  To: Ivo van Doorn; +Cc: netdev, rt2x00-devel
In-Reply-To: <200604280003.19645.IvDoorn@gmail.com>

On Fri, 28 Apr 2006 00:03:19 +0200, Ivo van Doorn wrote:
> Latest dscape patches have broken rt2x00,
> fix compile issues, and support the new features.

> @@ -2466,6 +2568,7 @@ rt2500pci_init_hw(struct rt2x00_pci *rt2
>  	hw->no_tkip_wmm_hwaccel = 1;
>  	hw->extra_hdr_room = 0;
>  	hw->device_strips_mic = 0;
> +	hw->monitor_during_oper = 1;

You should not set this to 1 if you support only one interface at a
time. Please add support for concurrent running of one regular (i.e.
STA, IBSS, AP, WDS) interface plus arbitrary number of monitor
interfaces or set monitor_during_oper to 0.

(Yes, I know, I definitely must reserve some time to write a
documentation about monitor mode support.)

Thanks,

-- 
Jiri Benc
SUSE Labs

^ permalink raw reply

* Re: [PATCH 7/32] rt2x00: make vals static
From: Ingo Oeser @ 2006-04-28 11:26 UTC (permalink / raw)
  To: Ivo van Doorn; +Cc: netdev, rt2x00-devel
In-Reply-To: <200604280002.58147.IvDoorn@gmail.com>

Sie schrieben:
> From: Ivo van Doorn <IvDoorn@gmail.com>
> 
> The vals[] arrays in *_init_hw_channels can be made
> static to optimize memory and reduce stack size.

What about static const? They are also constants, right?
But please try first, if this helps in terms of code size.

Regards

Ingo Oeser

^ permalink raw reply

* Re: IP1000 gigabit nic driver
From: David Gómez @ 2006-04-28 11:37 UTC (permalink / raw)
  To: Pekka Enberg; +Cc: David Vrabel, Francois Romieu, Linux-kernel, netdev
In-Reply-To: <84144f020604280358ie9990c7h399f4a5588e575f8@mail.gmail.com>

Hi Pekka,

On Apr 28 at 01:58:04, Pekka Enberg wrote:
> Needs some serious coding style cleanup and conversion to proper 2.6
> APIs for starters.

Ok, i could take care of that, and it's a good way of getting my hands
dirty with kernel programming ;). David, if it's ok to you i'll do the
cleanup thing.

What about 2.4/2.2 code? It's supposed to stay for compatibility
or it should be removed before submitting?

cheers,

-- 
David Gómez                                      Jabber ID: davidge@jabber.org

^ permalink raw reply

* Re: [PATCH 17/32] rt2x00: Put net_device structure in data_ring
From: Ingo Oeser @ 2006-04-28 11:45 UTC (permalink / raw)
  To: Ivo van Doorn; +Cc: netdev, rt2x00-devel
In-Reply-To: <200604280003.09753.IvDoorn@gmail.com>

Hi Ivo,

Ivo van Doorn wrote:
> diff -uprN wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2400pci.c wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2400pci.c
> --- wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2400pci.c	2006-04-27 21:48:21.000000000 +0200
> +++ wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2400pci.c	2006-04-27 21:49:08.000000000 +0200
> @@ -760,10 +760,8 @@ rt2400pci_write_tx_desc(
>  static void
>  rt2400pci_beacondone(void *data)
>  {
> -	struct data_ring	*ring = (struct data_ring*)data;
> -	struct rt2x00_pci	*rt2x00pci = (struct rt2x00_pci*)ring->dev;
> -	struct net_device	*net_dev = pci_get_drvdata(rt2x00pci->pci_dev);
> -	struct sk_buff		*skb;
> +	struct data_ring		*ring = (struct data_ring*)data;

No need for a cast here. 
In C you can cast from any struct pointer to void pointer and back
without problems.

> @@ -784,8 +782,8 @@ static void
>  rt2400pci_rxdone(void *data)
>  {
>  	struct data_ring	*ring = (struct data_ring*)data;

No need for casting.

> @@ -835,8 +834,8 @@ static void
>  rt2400pci_txdone(void *data)
>  {
>  	struct data_ring	*ring = (struct data_ring*)data;

No need for casting.

> diff -uprN wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2500pci.c wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2500pci.c
> --- wireless-dev-rt2x00/drivers/net/wireless/d80211/rt2x00/rt2500pci.c	2006-04-27 21:48:21.000000000 +0200
> +++ wireless-dev-rt2x00-patch/drivers/net/wireless/d80211/rt2x00/rt2500pci.c	2006-04-27 21:49:08.000000000 +0200
> @@ -834,10 +834,8 @@ rt2500pci_write_tx_desc(
>  static void
>  rt2500pci_beacondone(void *data)
>  {
> -	struct data_ring	*ring = (struct data_ring*)data;
> -	struct rt2x00_pci	*rt2x00pci = (struct rt2x00_pci*)ring->dev;
> -	struct net_device	*net_dev = pci_get_drvdata(rt2x00pci->pci_dev);
> -	struct sk_buff		*skb;
> +	struct data_ring		*ring = (struct data_ring*)data;

No need for casting.

Many more of them.

I guess you could just do a fgrep for "*ring = (struct data_ring*)data;"

Regards

Ingo Oeser

^ permalink raw reply

* Re: IP1000 gigabit nic driver
From: Pekka J Enberg @ 2006-04-28 11:51 UTC (permalink / raw)
  To: David Gómez; +Cc: David Vrabel, Francois Romieu, Linux-kernel, netdev
In-Reply-To: <20060428113755.GA7419@fargo>

On Apr 28 at 01:58:04, Pekka Enberg wrote:
> > Needs some serious coding style cleanup and conversion to proper 2.6
> > APIs for starters.

On Fri, 28 Apr 2006, David Gómezz wrote:
> Ok, i could take care of that, and it's a good way of getting my hands
> dirty with kernel programming ;). David, if it's ok to you i'll do the
> cleanup thing.
> 
> What about 2.4/2.2 code? It's supposed to stay for compatibility
> or it should be removed before submitting?

It's preferred not to have compatability cruft for 2.6 patch submissions, 
so I'd say kill them.

					Pekka

^ permalink raw reply

* Re: IP1000 gigabit nic driver
From: Pekka J Enberg @ 2006-04-28 11:59 UTC (permalink / raw)
  To: David Gómez; +Cc: David Vrabel, Francois Romieu, Linux-kernel, netdev
In-Reply-To: <20060428113755.GA7419@fargo>

Hi David,

On Fri, 28 Apr 2006, David Gómezz wrote:
> Ok, i could take care of that, and it's a good way of getting my hands
> dirty with kernel programming ;). David, if it's ok to you i'll do the
> cleanup thing.

Here are some suggestions for coding style cleanups:

  - Use Lindet for initial formatting
  - Kill use of LINUX_VERSION_CODE macro for compatability
  - Kill obfuscating macros. For example, IPG_DEV_KFREE_SKB and 
    IPG_DEVICE_TYPE.
  - Move changelogs outside of source files
  - Convert kmalloc/memset to kzalloc and kcalloc
  - Remove redundant typecasts
  - Remove dead code
  - Use dev_{dbg,err,info,warn} for logging
  - Remove unnecessary curly braces
  - Use proper naming convention for things like Length and pPHYParam
  - Convert macro functions to static inline functions for type safety

				Pekka

^ 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