public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>, passt-dev@passt.top
Subject: Re: [PATCH v3 7/9] checksum: introduce functions to compute the header part checksum for TCP/UDP
Date: Thu, 29 Feb 2024 19:49:09 +1100	[thread overview]
Message-ID: <ZeBFBQY6kL6fxmfS@zatzit> (raw)
In-Reply-To: <20240229080509.4f534831@elisabeth>

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

On Thu, Feb 29, 2024 at 08:05:09AM +0100, Stefano Brivio wrote:
> On Thu, 29 Feb 2024 11:38:53 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Feb 28, 2024 at 02:26:18PM +0100, Laurent Vivier wrote:
> > > On 2/19/24 04:08, David Gibson wrote:  
> > > > On Sat, Feb 17, 2024 at 04:07:23PM +0100, Laurent Vivier wrote:  
> > > >
> > > > [...]
> > > >
> > > > > +/**
> > > > > + * proto_ipv6_header_psum() - Calculates the partial checksum of an
> > > > > + * 			      IPv6 header for UDP or TCP
> > > > > + * @payload_len:	Payload length
> > > > > + * @proto:		Protocol number
> > > > > + * @saddr:		Source address
> > > > > + * @daddr:		Destination address
> > > > > + * Returns:	Partial checksum of the IPv6 header
> > > > > + */
> > > > > +uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol,
> > > > > +				struct in6_addr saddr, struct in6_addr daddr)  
> > > > 
> > > > Hrm, this is passing 2 16-byte IPv6 addresses by value, which might
> > > > not be what we want.  
> > > 
> > > The idea here is to avoid the pointer alignment problem (&ip6h->saddr and
> > > &ip6h->daddr can be misaligned).  
> > 
> > Ah, right.  That's a neat idea, but I'm not sure it really helps: I
> > think it will just move the misaligned access from inside the function
> > to the call site, where we try to marshal the parameter from something
> > unaligned.
> 
> I haven't tested this yet, but note that this is generally okay: the
> problem is *dereferencing* an unaligned pointer. But if you load memory
> from an aligned pointer, and extract a value from this memory, it's all
> fine.

Right, that's kind of what I'm getting at.  Assuming this value starts
in an unaligned buffer, then in order to pass this by value the caller
will need to load from that unaligned pointer.  AFAIK, the compiler
will base the type of loads only on the pointed to type, which isn't
changed whether we dereference in the caller or the callee.

> 
> Speaking MIPS, this is not safe on all CPU models:
> 
> 	la	$1, 1002   # s1 now contains the value 1002
> 	lw	$2, 0($1)  # load word from memory at 1002 + 0 into s2
> 
> but this is:
> 
> 	la	$1, 1000   # s1 now contains the value 1000
> 	la	$2, 1004   # s3 now contains the value 1004
> 	lw	$3, 0($1)  # load word from memory at 1000 + 0 into s3
> 	lw	$4, 0($3)  # load word from memory at 1004 + 0 into s4
> 	sll	$5, $3, 16 # 16-bit shift left s3 into s5
> 	srl	$6, $4, 16 # 16-bit shift right s4 into s6
> 	or	$2, $5, $6 # OR s5 and s6 into s2

Right, but I don't think merely moving the dereference to the caller
will necessarily induce the compiler to generate this rather than the
former.

> On x86, as far as I know, mov will digest the equivalent of the first
> version without issues.
> 
> > > Is it a better solution to copy the content of ip6h->saddr and ip6h->daddr
> > > to some local variables and then provide the pointers of the local variables
> > > to proto_ipv6_header_psum()?  
> > 
> > Honestly, I'm not sure.
> 
> I think it's pretty much the same. Let the compiler pass 16-byte
> variables by value, and it will generally do this for us, but only if
> needed.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2024-02-29  8:49 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-17 15:07 [PATCH v3 0/9] Add vhost-user support to passt (part 1) Laurent Vivier
2024-02-17 15:07 ` [PATCH v3 1/9] iov: add some functions to manage iovec Laurent Vivier
2024-02-19  2:45   ` David Gibson
2024-02-17 15:07 ` [PATCH v3 2/9] pcap: add pcap_iov() Laurent Vivier
2024-02-19  2:50   ` David Gibson
2024-02-17 15:07 ` [PATCH v3 3/9] checksum: align buffers Laurent Vivier
2024-02-17 15:07 ` [PATCH v3 4/9] checksum: add csum_iov() Laurent Vivier
2024-02-19  2:52   ` David Gibson
2024-02-17 15:07 ` [PATCH v3 5/9] util: move IP stuff from util.[ch] to ip.[ch] Laurent Vivier
2024-02-19  2:59   ` David Gibson
2024-02-17 15:07 ` [PATCH v3 6/9] checksum: use csum_ip4_header() in udp.c and tcp.c Laurent Vivier
2024-02-19  3:01   ` David Gibson
2024-02-29 16:24   ` Stefano Brivio
2024-02-29 23:10     ` David Gibson
2024-03-01  7:58       ` Stefano Brivio
2024-03-01 12:23         ` David Gibson
2024-03-04 17:04           ` Stefano Brivio
2024-02-17 15:07 ` [PATCH v3 7/9] checksum: introduce functions to compute the header part checksum for TCP/UDP Laurent Vivier
2024-02-19  3:08   ` David Gibson
2024-02-28 13:26     ` Laurent Vivier
2024-02-29  0:38       ` David Gibson
2024-02-29  7:05         ` Stefano Brivio
2024-02-29  8:49           ` David Gibson [this message]
2024-02-29  8:56             ` Stefano Brivio
2024-02-29 14:15               ` Stefano Brivio
2024-02-29 23:09                 ` David Gibson
2024-03-01  6:56                   ` Stefano Brivio
2024-03-04  1:54                     ` David Gibson
2024-03-04 11:00                       ` Stefano Brivio
2024-03-04 22:47                         ` Stefano Brivio
2024-03-06  5:09                           ` David Gibson
2024-03-08  0:08                             ` Stefano Brivio
2024-03-08  1:20                               ` David Gibson
2024-02-17 15:07 ` [PATCH v3 8/9] tap: make tap_update_mac() generic Laurent Vivier
2024-02-17 15:07 ` [PATCH v3 9/9] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers() Laurent Vivier
2024-02-19  3:14   ` David Gibson
2024-02-29 16:29   ` Stefano Brivio
2024-02-29 16:31 ` [PATCH v3 0/9] Add vhost-user support to passt (part 1) Stefano Brivio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZeBFBQY6kL6fxmfS@zatzit \
    --to=david@gibson.dropbear.id.au \
    --cc=lvivier@redhat.com \
    --cc=passt-dev@passt.top \
    --cc=sbrivio@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).