Willem de Bruijn wrote:
> 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@xxxxxxxxx>
>> ---
>> 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 */
>
Will do that, thanks for the review!
> 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);
>> + }
>> +}
>> +
