Received-SPF: pass (google.com: domain of linux-kernel+bounces-170231-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 06 May 2024 13:26:15 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Richard Gobert <richardbgobert@gmail.com>, 
 davem@davemloft.net, 
 edumazet@google.com, 
 willemdebruijn.kernel@gmail.com, 
 kuba@kernel.org, 
 pabeni@redhat.com, 
 dsahern@kernel.org, 
 alobakin@pm.me, 
 shuah@kernel.org, 
 netdev@vger.kernel.org, 
 linux-kernel@vger.kernel.org, 
 linux-kselftest@vger.kernel.org
Message-ID: <663912b7b9746_516de29445@willemb.c.googlers.com.notmuch>
In-Reply-To: <761374d3-1c76-4dc2-a4cc-7bd693deb453@gmail.com>
References: <20240506093550.128210-1-richardbgobert@gmail.com>
 <761374d3-1c76-4dc2-a4cc-7bd693deb453@gmail.com>
Subject: Re: [PATCH net-next v8 3/3] selftests/net: add flush id selftests
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

Richard Gobert wrote:
> Added flush id selftests to test different cases where DF flag is set or
> unset and id value changes in the following packets. All cases where the
> packets should coalesce or should not coalesce are tested.
> 
> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> ---
>  tools/testing/selftests/net/gro.c | 147 ++++++++++++++++++++++++++++++
>  1 file changed, 147 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
> index 353e1e867fbb..5dc7b539ccbf 100644
> --- a/tools/testing/selftests/net/gro.c
> +++ b/tools/testing/selftests/net/gro.c
> @@ -617,6 +617,123 @@ static void add_ipv6_exthdr(void *buf, void *optpkt, __u8 exthdr_type, char *ext
>  	iph->payload_len = htons(ntohs(iph->payload_len) + MIN_EXTHDR_SIZE);
>  }
>  
> +static void fix_ip4_checksum(struct iphdr *iph)
> +{
> +	iph->check = 0;
> +	iph->check = checksum_fold(iph, sizeof(struct iphdr), 0);
> +}
> +
> +static void send_flush_id_case(int fd, struct sockaddr_ll *daddr, int tcase)
> +{
> +	static char buf1[MAX_HDR_LEN + PAYLOAD_LEN];
> +	static char buf2[MAX_HDR_LEN + PAYLOAD_LEN];
> +	static char buf3[MAX_HDR_LEN + PAYLOAD_LEN];
> +	bool send_three = false;
> +	struct iphdr *iph1;
> +	struct iphdr *iph2;
> +	struct iphdr *iph3;
> +
> +	iph1 = (struct iphdr *)(buf1 + ETH_HLEN);
> +	iph2 = (struct iphdr *)(buf2 + ETH_HLEN);
> +	iph3 = (struct iphdr *)(buf3 + ETH_HLEN);
> +
> +	create_packet(buf1, 0, 0, PAYLOAD_LEN, 0);
> +	create_packet(buf2, PAYLOAD_LEN, 0, PAYLOAD_LEN, 0);
> +	create_packet(buf3, PAYLOAD_LEN * 2, 0, PAYLOAD_LEN, 0);
> +
> +	switch (tcase) {
> +	case 0: /* DF=1, Incrementing - should coalesce */
> +		iph1->frag_off |= htons(IP_DF);
> +		iph1->id = htons(8);
> +		fix_ip4_checksum(iph1);
> +
> +		iph2->frag_off |= htons(IP_DF);
> +		iph2->id = htons(9);
> +		fix_ip4_checksum(iph2);
> +		break;
> +
> +	case 1: /* DF=1, Fixed - should coalesce */
> +		iph1->frag_off |= htons(IP_DF);
> +		iph1->id = htons(8);
> +		fix_ip4_checksum(iph1);
> +
> +		iph2->frag_off |= htons(IP_DF);
> +		iph2->id = htons(8);
> +		fix_ip4_checksum(iph2);
> +		break;
> +
> +	case 2: /* DF=0, Incrementing - should coalesce */
> +		iph1->frag_off &= ~htons(IP_DF);
> +		iph1->id = htons(8);
> +		fix_ip4_checksum(iph1);
> +
> +		iph2->frag_off &= ~htons(IP_DF);
> +		iph2->id = htons(9);
> +		fix_ip4_checksum(iph2);
> +		break;
> +
> +	case 3: /* DF=0, Fixed - should not coalesce */
> +		iph1->frag_off &= ~htons(IP_DF);
> +		iph1->id = htons(8);
> +		fix_ip4_checksum(iph1);
> +
> +		iph2->frag_off &= ~htons(IP_DF);
> +		iph2->id = htons(8);
> +		fix_ip4_checksum(iph2);
> +		break;
> +
> +	case 4: /* DF=1, two packets incrementing, and one fixed - should
> +		 * coalesce only the first two packets
> +		 */
> +		iph1->frag_off |= htons(IP_DF);
> +		iph1->id = htons(8);
> +		fix_ip4_checksum(iph1);
> +
> +		iph2->frag_off |= htons(IP_DF);
> +		iph2->id = htons(9);
> +		fix_ip4_checksum(iph2);
> +
> +		iph3->frag_off |= htons(IP_DF);
> +		iph3->id = htons(9);
> +		fix_ip4_checksum(iph3);
> +		send_three = true;
> +		break;
> +
> +	case 5: /* DF=1, two packets fixed, and one incrementing - should
> +		 * coalesce only the first two packets
> +		 */
> +		iph1->frag_off |= htons(IP_DF);
> +		iph1->id = htons(8);
> +		fix_ip4_checksum(iph1);
> +
> +		iph2->frag_off |= htons(IP_DF);
> +		iph2->id = htons(8);
> +		fix_ip4_checksum(iph2);
> +
> +		iph3->frag_off |= htons(IP_DF);
> +		iph3->id = htons(9);
> +		fix_ip4_checksum(iph3);
> +		send_three = true;
> +		break;
> +	}

Consider moving the fix_ip4_checksum calls out of the switch to reduce
duplication.

> +
> +	write_packet(fd, buf1, total_hdr_len + PAYLOAD_LEN, daddr);
> +	write_packet(fd, buf2, total_hdr_len + PAYLOAD_LEN, daddr);
> +
> +	if (send_three)
> +		write_packet(fd, buf3, total_hdr_len + PAYLOAD_LEN, daddr);
> +}
> +
> +static void test_flush_id(int fd, struct sockaddr_ll *daddr, char *fin_pkt)
> +{
> +	for (int i = 0; i < 6; i++) {

Please avoid unnamed magic constants. Something like

const int num_flush_id_cases = 6;	/* See switch in send_flush_id_case */

Or even define an enum with named tests and and _MAX val. It's
verbose, but helpful to readers.

> +		sleep(1);
> +		send_flush_id_case(fd, daddr, i);
> +		sleep(1);
> +		write_packet(fd, fin_pkt, total_hdr_len, daddr);
> +	}
> +}
> +