Zerocopy implementation issues

Hi,

Can someone please explain to me the rationale behind the zerocopy implementation that has appeared in 2.4.4 please?

The reason I ask is that even on x86, it seems to me to be extremely silly to have the expense of doing unaligned checksumming for the gain of zerocopy.

Just think - if you did checksumming on aligned word boundaries you could be far faster!

(Yes, you guessed it, its broken on ARM, and is going to make the networking layer pig slow if we keep the current implementation as it stands due to the phenominal amount of exceptions it _will_ generate - 1 exception per word in a packet).

I'm still investigating the source of the networking corruptions I'm seeing, but its looking like the above is the reason (data is being corrupted on TCP send).

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

You've had at least 2 months to ask this question. Like I have asked several others I am going to ask you, why you are complaining now?

I see you merged your ARM updates to the AC tree, which is the same place where the zerocopy stuff went in 2 months for _testing_, why didn't you notice it? I was posting zerocopy patches on almost a daily basis at one point, and I mentioned in each of those patch releases here on linux-kernel that the intent was to merge this to Linus. Why didn't you give it a go to make sure it worked on ARM?

PPC people, along with several other ports, did this and contacted me to tell me things were OK.

If the ARM checksum routines do not take care of unaligned source and/or dest buffers, it is completely broken. Every other port handles this, ARM has byte/halfword/word loads and stores so it can handle this just fine. There is no excuse for this.

And your performance logic is broken too, the thing that is requiring byte aligned source/dest buffers is eliminating an entire copy across loopback, and in the sendfile case is copying the data directly from the page cache into the receiver's userspace buffer.

So you're saying that 2 aligned copies are faster than 1 that starts on a byte boundary?

ARM's checksum routines are what is broken, not the zerocopy patches. Note this is a totally different case than the unaligned loads/stores done to protocol headers that ARM had such trouble with in the past.

These unaligned case happen today, the user can give the kernel on a send() any alignment it so chooses. And the checksum routine is going to operate on that user source buffer, whether it is byte or word aligned. So don't make it seem like this is some new situation, becuase it simply isn't.

The zerocopy stuff has actually improved latency and bandwidth, BTW. Linus required that it not deteriorate performance for existing cases, and we verified that it did not on at least 3 platforms (x86, Alpha, and sparc64).

In short, fix the ARM checksum code. The kernel is just fine.

Later,
David S. Miller
[email protected]

Of course not.

Hang on - you mean that this isn't equivalent to:

	dst & 2

?

If it is, I don't see how his code can work, but then I'm not an expert at reading x86 code unlike you!

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

Ok, I see precisely what's going on here now, shame you didn't explain about these csum_add stuff in your first mail on this subject, and we could've saved going down this path.

I'll fix up the ARM code, but its not going to be nice.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

David,

Would it be acceptable to have csum_block_* in the architecture specific code?

Firstly, architecture specific code can optimise them more efficiently, and secondly it will prevent checksum rotations in the architecture specific code which will only get undone by the csum_block_* code.

Or am I missing something?

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

csum_block_*() has nothing to do with checksumming buffers, it 2's complement adds two integers passed as arguments based upon the offset of one of the buffers (this decides if one of the csums needs to be byte swapped before the 2's complement addition to get a correct result).

That was the point of my mail, the last_byte_was_odd code had nothing to do with what your checksum code needs to be doing, never did and never will. Your premise was that the last_byte_was_odd code "proved" that the csum_partial_copy destination buffer could not be byte aligned, and I tried to show that the last_byte_was_odd code had nothing to do with whether that was allowed or not.

Your csum_partial_copy*() code needs to handle unaligned destination buffers, period.

I understand that you are frustruated about this and it requires you to touch some delicate assembly. But I'm going to be blunt and say "tough", because everyone has to implement this correctly. Just do it and get it over with.

Later,
David S. Miller
[email protected]

I'm doing it _NOW_, but I'm having to rotate the checksum at the end if dst & 1, only to have it unrotated in an inefficient manner in csum_block_*. Seems a bit of a waste of CPU cycles.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

This is certainly the kind of enhancement we can make, where possible. No problem.

But I currently can't see how you would apply it even in the net/ipv4/tcp.c:tcp_copy_to_page() case. Look:

page_address(page) + off

This determines whether you need to rotate at the end in your checksum code, right? We know pages are aligned so 'off' is all that is relevant for that determination.

Later we use:

	skb->len

to determine the behavior of csum_block_add() (to byte swap or not).

There is no connection between 'off' and 'skb->len' so I don't see how in this case we could make use of your optimization.

Later,
David S. Miller
[email protected]