Zerocopy implementation issues

Zerocopy implementation issues

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Zerocopy implementation issues

[David S. Miller]

Russell King writes:
 > Can someone please explain to me the rationale behind the zerocopy
 > implementation that has appeared in 2.4.4 please?

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.

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

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
davem@redhat.com

Re: Zerocopy implementation issues

[Russell King]

On Sun, Apr 29, 2001 at 01:18:43AM -0700, David S. Miller wrote:
> When I don't have the time, my port tends to break too.
> This isn't news to you is it?

Of course not.

>         testl $2, %edi                  # Check alignment.

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Zerocopy implementation issues

[Russell King]

On Sun, Apr 29, 2001 at 01:18:43AM -0700, David S. Miller wrote:
> Occaisionally I find that sparc64 is making a gross error or invalid
> assumption, and I accept this and fix it up.

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Zerocopy implementation issues

[Russell King]

On Sun, Apr 29, 2001 at 10:17:39AM +0100, Russell King wrote:
> On Sun, Apr 29, 2001 at 01:18:43AM -0700, David S. Miller wrote:
> > Occaisionally I find that sparc64 is making a gross error or invalid
> > assumption, and I accept this and fix it up.
> 
> 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.

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Zerocopy implementation issues

[David S. Miller]

Russell King writes:
 > Or am I missing something?

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
davem@redhat.com

Re: Zerocopy implementation issues

[Russell King]

On Sun, Apr 29, 2001 at 04:06:16AM -0700, David S. Miller wrote:
> 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.

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Zerocopy implementation issues

[David S. Miller]

Russell King writes:
 > 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.

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
davem@redhat.com