On Thu, Dec 21, 2017 at 1:16 AM, Yunsheng Lin <linyunsh...@huawei.com> wrote: > Hi, Alexander > > On 2017/12/21 0:24, Alexander Duyck wrote: >> On Wed, Dec 20, 2017 at 1:09 AM, Yunsheng Lin <linyunsh...@huawei.com> wrote: >>> Hi, all >>> I have some doubt about NAPI_GRO_CB(skb)->is_atomic when >>> analyzing the tcpv4 gro process: >>> >>> Firstly we set NAPI_GRO_CB(skb)->is_atomic to 1 in dev_gro_receive: >>> https://elixir.free-electrons.com/linux/v4.15-rc4/source/net/core/dev.c#L4838 >>> >>> And then in inet_gro_receive, we check the NAPI_GRO_CB(skb)->is_atomic >>> before setting NAPI_GRO_CB(skb)->is_atomic according to IP_DF bit in the ip >>> header: >>> https://elixir.free-electrons.com/linux/v4.15-rc4/source/net/ipv4/af_inet.c#L1319 >>> >>> struct sk_buff **inet_gro_receive(struct sk_buff **head, struct sk_buff >>> *skb) >>> { >>> ..................... >>> for (p = *head; p; p = p->next) { >>> ........................ >>> >>> /* If the previous IP ID value was based on an atomic >>> * datagram we can overwrite the value and ignore it. >>> */ >>> if (NAPI_GRO_CB(skb)->is_atomic) //we >>> check it here >>> NAPI_GRO_CB(p)->flush_id = flush_id; >>> else >>> NAPI_GRO_CB(p)->flush_id |= flush_id; >>> } >>> >>> NAPI_GRO_CB(skb)->is_atomic = !!(iph->frag_off & htons(IP_DF)); >>> //we set it here >>> NAPI_GRO_CB(skb)->flush |= flush; >>> skb_set_network_header(skb, off); >>> ................................ >>> } >>> >>> My question is whether we should check the NAPI_GRO_CB(skb)->is_atomic or >>> NAPI_GRO_CB(p)->is_atomic? >>> If we should check NAPI_GRO_CB(skb)->is_atomic, then maybe it is >>> unnecessary because it is alway true. >>> If we should check NAPI_GRO_CB(p)->is_atomic, maybe there is a bug here. >>> >>> So what is the logic here? I am just start analyzing the gro, maybe I miss >>> something obvious here. >> >> The logic there is to address the multiple IP header case where there >> are 2 or more IP headers due to things like VXLAN or GRE tunnels. So >> what will happen is that an outer IP header will end up being sent >> with DF not set and will clear the is_atomic value then we want to OR >> in the next header that is applied. It defaults to assignment on >> is_atomic because the first IP header will encounter flush_id with no >> previous configuration occupying it. > > I see your point now. > > But for the same flow of tunnels packet, the outer and inner ip header must > have the same fixed id or increment id? > > For example, if we have a flow of tunnels packet which has fixed id in outer > header and increment id in inner header(the inner header does have DF flag > set): > > 1. For the first packet, NAPI_GRO_CB(skb)->is_atomic will be set to zero when > inet_gro_receive is processing the inner ip header. > > 2. For the second packet, when inet_gro_receive is processing the outer ip > header > which has a fixed id, NAPI_GRO_CB(p)->is_atomic is zero according to [1], so > NAPI_GRO_CB(p)->flush_id will be set to 0xFFFF, then the second packet will > not > be merged to first packet in tcp_gro_receive.
I'm not sure how valid your case here is. The is_atomic is only really meant to apply to the inner-most header. In the case of TCP the inner-most header should almost always have the DF bit set which means the inner-most is almost always atomic. > I thought outer ip header could have a fixed id while inner ip header could > have a increment id. Do I miss something here? You have it backwards. The innermost will have DF bit set so it can be fixed, the outer-most will in many cases not since it is usually UDP and as such it will likely need to increment. >> >> The part I am not sure about is if we should be using assignment for >> is_atomic or using an "&=" to clear the bit and leave it cleared. > > I am not sure I understood you here. is_atomic is a bit field, why do you > want to use "&="? Actually that was my mind kind of wandering. It has been a while since I looked at this code and the use of &= wouldn't be appropriate since is_atomic should only apply to the innermost header. Basically the only acceptable combinations for is_atomic and flush_id are false with 0, or true with 1. We can't have a fixed outer header value if DF is not set. Hope that helps to clarify things. - Alex