Hi Toke, Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master] url: https://github.com/0day-ci/linux/commits/Toke-H-iland-J-rgensen/sched-Add-Common-Applications-Kept-Enhanced-cake-qdisc/20180503-073002 coccinelle warnings: (new ones prefixed by >>) >> net/sched/sch_cake.c:1047:6-13: ERROR: PTR_ERR applied after initialization >> to constant on line 822 vim +1047 net/sched/sch_cake.c 812 813 static struct sk_buff *cake_ack_filter(struct cake_sched_data *q, 814 struct cake_flow *flow) 815 { 816 bool thisconn_redundant_seen = false, thisconn_seen_last = false; 817 bool aggressive = q->ack_filter == CAKE_ACK_AGGRESSIVE; 818 bool otherconn_ack_seen = false; 819 struct sk_buff *skb_check, *skb_check_prev; 820 struct sk_buff *otherconn_checked_to = NULL; 821 struct sk_buff *thisconn_checked_to = NULL; 822 struct sk_buff *thisconn_ack = NULL; 823 const struct ipv6hdr *ipv6h, *ipv6h_check; 824 const struct tcphdr *tcph, *tcph_check; 825 const struct iphdr *iph, *iph_check; 826 const struct sk_buff *skb; 827 struct ipv6hdr _iph, _iph_check; 828 struct tcphdr _tcph_check; 829 unsigned char _tcph[64]; /* need to hold maximum hdr size */ 830 int seglen; 831 832 /* no other possible ACKs to filter */ 833 if (flow->head == flow->tail) 834 return NULL; 835 836 skb = flow->tail; 837 tcph = cake_get_tcphdr(skb, _tcph, sizeof(_tcph)); 838 iph = cake_get_iphdr(skb, &_iph); 839 if (!tcph) 840 return NULL; 841 842 /* the 'triggering' packet need only have the ACK flag set. 843 * also check that SYN is not set, as there won't be any previous ACKs. 844 */ 845 if ((tcp_flag_word(tcph) & 846 (TCP_FLAG_ACK | TCP_FLAG_SYN)) != TCP_FLAG_ACK) 847 return NULL; 848 849 /* the 'triggering' ACK is at the end of the queue, 850 * we have already returned if it is the only packet in the flow. 851 * stop before last packet in queue, don't compare trigger ACK to itself 852 * start where we finished last time if recorded in ->ackcheck 853 * otherwise start from the the head of the flow queue. 854 */ 855 skb_check_prev = flow->ackcheck; 856 skb_check = flow->ackcheck ?: flow->head; 857 858 while (skb_check->next) { 859 bool pure_ack, thisconn; 860 861 /* don't increment if at head of flow queue (_prev == NULL) */ 862 if (skb_check_prev) { 863 skb_check_prev = skb_check; 864 skb_check = skb_check->next; 865 if (!skb_check->next) 866 break; 867 } else { 868 skb_check_prev = ERR_PTR(-1); 869 } 870 871 iph_check = cake_get_iphdr(skb_check, &_iph_check); 872 tcph_check = cake_get_tcphdr(skb_check, &_tcph_check, 873 sizeof(_tcph_check)); 874 875 if (!tcph_check || iph->version != iph_check->version) 876 continue; 877 878 if (iph->version == 4) { 879 seglen = ntohs(iph_check->tot_len) - 880 (4 * iph_check->ihl); 881 882 thisconn = (iph_check->saddr == iph->saddr && 883 iph_check->daddr == iph->daddr); 884 } else if (iph->version == 6) { 885 ipv6h = (struct ipv6hdr *)iph; 886 ipv6h_check = (struct ipv6hdr *)iph_check; 887 seglen = ntohs(ipv6h_check->payload_len); 888 889 thisconn = (!ipv6_addr_cmp(&ipv6h_check->saddr, 890 &ipv6h->saddr) && 891 !ipv6_addr_cmp(&ipv6h_check->daddr, 892 &ipv6h->daddr)); 893 } else { 894 WARN_ON(1); /* shouldn't happen */ 895 continue; 896 } 897 898 /* stricter criteria apply to ACKs that we may filter 899 * 3 reserved flags must be unset to avoid future breakage 900 * ECE/CWR/NS can be safely ignored 901 * ACK must be set 902 * All other flags URG/PSH/RST/SYN/FIN must be unset 903 * 0x0FFF0000 = all TCP flags (confirm ACK=1, others zero) 904 * 0x01C00000 = NS/CWR/ECE (safe to ignore) 905 * 0x0E3F0000 = 0x0FFF0000 & ~0x01C00000 906 * must be 'pure' ACK, contain zero bytes of segment data 907 * options are ignored 908 */ 909 if ((tcp_flag_word(tcph_check) & 910 (TCP_FLAG_ACK | TCP_FLAG_SYN)) != TCP_FLAG_ACK) 911 continue; 912 913 else if (((tcp_flag_word(tcph_check) & 914 cpu_to_be32(0x0E3F0000)) != TCP_FLAG_ACK) || 915 ((seglen - __tcp_hdrlen(tcph_check)) != 0)) 916 pure_ack = false; 917 918 else 919 pure_ack = true; 920 921 /* if we find an ACK belonging to a different connection 922 * continue checking for other ACKs this round however 923 * restart checking from the other connection next time. 924 */ 925 if (thisconn && (tcph_check->source != tcph->source || 926 tcph_check->dest != tcph->dest)) 927 thisconn = false; 928 929 /* new ack sequence must be greater 930 */ 931 if (thisconn && 932 ((int32_t)(ntohl(tcph_check->ack_seq) - 933 ntohl(tcph->ack_seq)) > 0)) 934 continue; 935 936 /* DupACKs with an equal sequence number shouldn't be filtered, 937 * but we can filter if the triggering packet is a SACK 938 */ 939 if (thisconn && 940 (ntohl(tcph_check->ack_seq) == ntohl(tcph->ack_seq))) { 941 /* inspired by tcp_parse_options in tcp_input.c */ 942 bool sack = false; 943 int length = __tcp_hdrlen(tcph) - sizeof(struct tcphdr); 944 const u8 *ptr = (const u8 *)(tcph + 1); 945 946 while (length > 0) { 947 int opcode = *ptr++; 948 int opsize; 949 950 if (opcode == TCPOPT_EOL) 951 break; 952 if (opcode == TCPOPT_NOP) { 953 length--; 954 continue; 955 } 956 opsize = *ptr++; 957 if (opsize < 2 || opsize > length) 958 break; 959 if (opcode == TCPOPT_SACK) { 960 sack = true; 961 break; 962 } 963 ptr += opsize - 2; 964 length -= opsize; 965 } 966 if (!sack) 967 continue; 968 } 969 970 /* somewhat complicated control flow for 'conservative' 971 * ACK filtering that aims to be more polite to slow-start and 972 * in the presence of packet loss. 973 * does not filter if there is one 'redundant' ACK in the queue. 974 * 'data' ACKs won't be filtered but do count as redundant ACKs. 975 */ 976 if (thisconn) { 977 thisconn_seen_last = true; 978 /* if aggressive and this is a data ack we can skip 979 * checking it next time. 980 */ 981 thisconn_checked_to = (aggressive && !pure_ack) ? 982 skb_check : skb_check_prev; 983 /* the first pure ack for this connection. 984 * record where it is, but only break if aggressive 985 * or already seen data ack from the same connection 986 */ 987 if (pure_ack && !thisconn_ack) { 988 thisconn_ack = skb_check_prev; 989 if (aggressive || thisconn_redundant_seen) 990 break; 991 /* data ack or subsequent pure ack */ 992 } else { 993 thisconn_redundant_seen = true; 994 /* this is the second ack for this connection 995 * break to filter the first pure ack 996 */ 997 if (thisconn_ack) 998 break; 999 } 1000 /* track packets from non-matching tcp connections that will 1001 * need evaluation on the next run. 1002 * if there are packets from both the matching connection and 1003 * others that requre checking next run, track which was updated 1004 * last and return the older of the two to ensure full coverage. 1005 * if a non-matching pure ack has been seen, cannot skip any 1006 * further on the next run so don't update. 1007 */ 1008 } else if (!otherconn_ack_seen) { 1009 thisconn_seen_last = false; 1010 if (pure_ack) { 1011 otherconn_ack_seen = true; 1012 /* if aggressive we don't care about old data, 1013 * start from the pure ack. 1014 * otherwise if there is a previous data ack, 1015 * start checking from it next time. 1016 */ 1017 if (aggressive || !otherconn_checked_to) 1018 otherconn_checked_to = skb_check_prev; 1019 } else { 1020 otherconn_checked_to = aggressive ? 1021 skb_check : skb_check_prev; 1022 } 1023 } 1024 } 1025 1026 /* skb_check is reused at this point 1027 * it is the pure ACK to be filtered (if any) 1028 */ 1029 skb_check = NULL; 1030 1031 /* next time start checking from the older/nearest to head of unfiltered 1032 * but important tcp packets from this connection and other connections. 1033 * if none seen, start after the last packet evaluated in the loop. 1034 */ 1035 if (thisconn_checked_to && otherconn_checked_to) 1036 flow->ackcheck = thisconn_seen_last ? 1037 otherconn_checked_to : thisconn_checked_to; 1038 else if (thisconn_checked_to) 1039 flow->ackcheck = thisconn_checked_to; 1040 else if (otherconn_checked_to) 1041 flow->ackcheck = otherconn_checked_to; 1042 else 1043 flow->ackcheck = skb_check_prev; 1044 1045 /* if filtering, remove the pure ACK from the flow queue */ 1046 if (thisconn_ack && (aggressive || thisconn_redundant_seen)) { > 1047 if (PTR_ERR(thisconn_ack) == -1) { 1048 skb_check = flow->head; 1049 flow->head = flow->head->next; 1050 } else { 1051 skb_check = thisconn_ack->next; 1052 thisconn_ack->next = thisconn_ack->next->next; 1053 } 1054 } 1055 1056 /* we just filtered that ack, fix up the list */ 1057 if (flow->ackcheck == skb_check) 1058 flow->ackcheck = thisconn_ack; 1059 /* check the entire flow queue next time */ 1060 if (PTR_ERR(flow->ackcheck) == -1) 1061 flow->ackcheck = NULL; 1062 1063 return skb_check; 1064 } 1065 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
