Re: [QUESTION] Doubt about NAPI_GRO_CB(skb)->is_atomic in tcpv4 gro process

On Wed, Dec 20, 2017 at 1:09 AM, Yunsheng Lin [off-list ref] 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.

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
don't know if there has been much testing of multiple levels of tunnel
header.

Thanks.

- Alex