Re: [RFC] tcp: Replace TCP buffer structure by an iovec array

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 13 Mar 2024 16:20:12 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 3/13/24 12:37, Stefano Brivio wrote:
> > On Mon, 11 Mar 2024 14:33:56 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> ...
> >> @@ -445,133 +413,78 @@ struct tcp_buf_seq_update {
> >>   };
> >>   
> >>   /* Static buffers */
> >> -
> >>   /**
> >> - * tcp4_l2_buf_t - Pre-cooked IPv4 packet buffers for tap connections
> >> - * @pad:	Align TCP header to 32 bytes, for AVX2 checksum calculation only
> >> - * @taph:	Tap-level headers (partially pre-filled)
> >> - * @iph:	Pre-filled IP header (except for tot_len and saddr)
> >> - * @uh:		Headroom for TCP header
> >> - * @data:	Storage for TCP payload
> >> + * tcp_l2_flags_t - TCP header and data to send option flags  
> > 
> > 'struct tcp_l2_flags_t - ...', by the way (pre-existing, I see).
> >   
> >> + * @th:		TCP header
> >> + * @opts	TCP option flags
> >>    */
> >> -static struct tcp4_l2_buf_t {
> >> -#ifdef __AVX2__
> >> -	uint8_t pad[26];	/* 0, align th to 32 bytes */
> >> -#else
> >> -	uint8_t pad[2];		/*	align iph to 4 bytes	0 */
> >> -#endif
> >> -	struct tap_hdr taph;	/* 26				2 */
> >> -	struct iphdr iph;	/* 44				20 */
> >> -	struct tcphdr th;	/* 64				40 */
> >> -	uint8_t data[MSS4];	/* 84				60 */
> >> -				/* 65536			65532 */
> >> +struct tcp_l2_flags_t {
> >> +	struct tcphdr th;
> >> +	char opts[OPT_MSS_LEN + OPT_WS_LEN + 1];
> >> +};  
> > 
> > This should still be aligned (when declared below) with:
> > 
> > #ifdef __AVX2__
> > } __attribute__ ((packed, aligned(32)))
> > #else
> > } __attribute__ ((packed, aligned(__alignof__(unsigned int))))
> > #endif
> > 
> > because yes, now you get the unsigned int alignment for free, but the
> > 32-byte boundary for AVX2 (checksums) not really, so that needs to be
> > there, and then for clarity I would keep the explicit attribute for the
> > non-AVX2 case.
> > 
> > By the way, I guess you could still combine the definition and
> > declaration as it was done earlier.  
> 
> We can't combine the definition and declaration because the same definition is used with 
> IPv4 and IPv6 TCP arrays declaration.

Ah, sorry, of course!

By the way of that, I wonder: would it then be possible to have a
single copy of payload buffers, that's shared for IPv4 and IPv6? We
would probably need to introduce a separate counter, on top of
tcp[46]_l2_buf_used (which could become tcp[46]_l2_hdr_used).

Well, probably not in scope for this change anyway.

> I'm not sure: the __attribute__ must be used on the structure definition or on the array 
> of structures declaration?

__attribute__ can be used on definitions or declarations depending on
the type of attribute itself -- here it would be on the definition,
because it affects the definition of the data type itself.

Strictly speaking, you can use it on the declaration too, but gcc will
ignore it:

$ cat attr.c
struct a {
    int b;
};

int main()
{
    struct a c __attribute__ ((packed, aligned(32)));
    return 0;
}

$ gcc -o attr attr.c
attr.c: In function ‘main’:
attr.c:7:12: warning: ‘packed’ attribute ignored [-Wattributes]
    7 |     struct a c __attribute__ ((packed, aligned(32)));
      |            

-- 
Stefano