netdev - Re: [PATCH v2 09/14] net: wwan: t7xx: Add WWAN network interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5755abe9-7b3c-0361-4eea-e0c125811eae@linux.intel.com>
Date:   Tue, 30 Nov 2021 22:06:01 -0800
From:   "Martinez, Ricardo" <ricardo.martinez@...ux.intel.com>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc:     netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Johannes Berg <johannes@...solutions.net>,
        Loic Poulain <loic.poulain@...aro.org>,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        chandrashekar.devegowda@...el.com,
        Intel Corporation <linuxwwan@...el.com>,
        chiranjeevi.rapolu@...ux.intel.com, haijun.liu@...iatek.com,
        amir.hanania@...el.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        dinesh.sharma@...el.com, eliot.lee@...el.com,
        mika.westerberg@...ux.intel.com, moises.veleta@...el.com,
        pierre-louis.bossart@...el.com, muralidharan.sethuraman@...el.com,
        Soumya.Prakash.Mishra@...el.com, sreehari.kancharla@...el.com,
        suresh.nagaraj@...el.com
Subject: Re: [PATCH v2 09/14] net: wwan: t7xx: Add WWAN network interface


On 11/6/2021 11:08 AM, Sergey Ryazanov wrote:
> On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
> <ricardo.martinez@...ux.intel.com> wrote:
>> Creates the Cross Core Modem Network Interface (CCMNI) which implements
>> the wwan_ops for registration with the WWAN framework, CCMNI also
>> implements the net_device_ops functions used by the network device.
>> Network device operations include open, close, start transmission, TX
>> timeout, change MTU, and select queue.
>>
[skipped]
>> +static enum txq_type get_txq_type(struct sk_buff *skb)
>> +{
>> +       u32 total_len, payload_len, l4_off;
>> +       bool tcp_syn_fin_rst, is_tcp;
>> +       struct ipv6hdr *ip6h;
>> +       struct tcphdr *tcph;
>> +       struct iphdr *ip4h;
>> +       u32 packet_type;
>> +       __be16 frag_off;
>> +
>> +       packet_type = skb->data[0] & SBD_PACKET_TYPE_MASK;
>> +       if (packet_type == IPV6_VERSION) {
>> +               ip6h = (struct ipv6hdr *)skb->data;
>> +               total_len = sizeof(struct ipv6hdr) + ntohs(ip6h->payload_len);
>> +               l4_off = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &ip6h->nexthdr, &frag_off);
>> +               tcph = (struct tcphdr *)(skb->data + l4_off);
>> +               is_tcp = ip6h->nexthdr == IPPROTO_TCP;
>> +               payload_len = total_len - l4_off - (tcph->doff << 2);
>> +       } else if (packet_type == IPV4_VERSION) {
>> +               ip4h = (struct iphdr *)skb->data;
>> +               tcph = (struct tcphdr *)(skb->data + (ip4h->ihl << 2));
>> +               is_tcp = ip4h->protocol == IPPROTO_TCP;
>> +               payload_len = ntohs(ip4h->tot_len) - (ip4h->ihl << 2) - (tcph->doff << 2);
>> +       } else {
>> +               return TXQ_NORMAL;
>> +       }
>> +
>> +       tcp_syn_fin_rst = tcph->syn || tcph->fin || tcph->rst;
>> +       if (is_tcp && !payload_len && !tcp_syn_fin_rst)
>> +               return TXQ_FAST;
>> +
>> +       return TXQ_NORMAL;
>> +}
> I am wondering how much modem performance has improved with this
> optimization compared to the performance loss on each packet due to
> the cache miss? Do you have any measurement results?

No performance gains observed in the latest tests, this is going to be 
removed for the

next iteration.

>> +static u16 ccmni_select_queue(struct net_device *dev, struct sk_buff *skb,
>> +                             struct net_device *sb_dev)
>> +{
>> +       struct ccmni_instance *ccmni;
>> +
>> +       ccmni = netdev_priv(dev);
>> +
>> +       if (ccmni->ctlb->capability & NIC_CAP_DATA_ACK_DVD)
>> +               return get_txq_type(skb);
>> +
>> +       return TXQ_NORMAL;
>> +}
>> +
>> +static int ccmni_open(struct net_device *dev)
>> +{
>> +       struct ccmni_instance *ccmni;
>> +
>> +       ccmni = wwan_netdev_drvpriv(dev);
>
[skipped]
>> +       skb_set_mac_header(skb, -ETH_HLEN);
>> +       skb_reset_network_header(skb);
>> +       skb->dev = dev;
>> +       if (pkt_type == IPV6_VERSION)
>> +               skb->protocol = htons(ETH_P_IPV6);
>> +       else
>> +               skb->protocol = htons(ETH_P_IP);
>> +
>> +       skb_len = skb->len;
>> +
>> +       netif_rx_any_context(skb);
> Did you consider using NAPI for the packet Rx path? This should
> improve Rx performance.
Yes, NAPI implementation is in the plan.
>> +       dev->stats.rx_packets++;
>> +       dev->stats.rx_bytes += skb_len;
>> +}
> [skipped]
>
>> diff --git a/drivers/net/wwan/t7xx/t7xx_netdev.h b/drivers/net/wwan/t7xx/t7xx_netdev.h
>> ...
>> +#define CCMNI_TX_QUEUE         1000
> Is this a really carefully selected queue depth limit, or just an
> arbitrary value? If the last one, then feel free to use  the
> DEFAULT_TX_QUEUE_LEN macro.
Changing this to DEFAULT_TX_QUEUE_LEN for the next iteration
>> ..
>> +#define IPV4_VERSION           0x40
>> +#define IPV6_VERSION           0x60
> Just curious why the _VERSION suffix? Why not, for example, PKT_TYPE_ prefix?
Nothing special about _VERSION, but it does look a bit weird, will use 
PKT_TYPE_  as suggested
> --
> Sergey
Ricardo