On 11/6/2021 11:05 AM, Sergey Ryazanov wrote:

On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
[off-list ref] wrote:

quoted

Registers the t7xx device driver with the kernel. Setup all the core
components: PCIe layer, Modem Host Cross Core Interface (MHCCIF),
modem control operations, modem state machine, and build
infrastructure.

* PCIe layer code implements driver probe and removal.
* MHCCIF provides interrupt channels to communicate events
   such as handshake, PM and port enumeration.
* Modem control implements the entry point for modem init,
   reset and exit.
* The modem status monitor is a state machine used by modem control
   to complete initialization and stop. It is used also to propagate
   exception events reported by other components.

[skipped]

quoted

  drivers/net/wwan/t7xx/t7xx_monitor.h       | 144 +++++
...
  drivers/net/wwan/t7xx/t7xx_state_monitor.c | 598 +++++++++++++++++++++

Out of curiosity, why is this file called t7xx_state_monitor.c, while
the corresponding header file is called simply t7xx_monitor.h? Are any
other monitors planed?

[skipped]

No other monitors, I'll rename it to make it consistent.

[skipped]

quoted

diff --git a/drivers/net/wwan/t7xx/t7xx_skb_util.c b/drivers/net/wwan/t7xx/t7xx_skb_util.c

...
+static struct sk_buff *alloc_skb_from_pool(struct skb_pools *pools, size_t size)
+{
+       if (size > MTK_SKB_4K)
+               return ccci_skb_dequeue(pools->reload_work_queue, &pools->skb_pool_64k);
+       else if (size > MTK_SKB_16)
+               return ccci_skb_dequeue(pools->reload_work_queue, &pools->skb_pool_4k);
+       else if (size > 0)
+               return ccci_skb_dequeue(pools->reload_work_queue, &pools->skb_pool_16);
+
+       return NULL;
+}
+
+static struct sk_buff *alloc_skb_from_kernel(size_t size, gfp_t gfp_mask)
+{
+       if (size > MTK_SKB_4K)
+               return __dev_alloc_skb(MTK_SKB_64K, gfp_mask);
+       else if (size > MTK_SKB_1_5K)
+               return __dev_alloc_skb(MTK_SKB_4K, gfp_mask);
+       else if (size > MTK_SKB_16)
+               return __dev_alloc_skb(MTK_SKB_1_5K, gfp_mask);
+       else if (size > 0)
+               return __dev_alloc_skb(MTK_SKB_16, gfp_mask);
+
+       return NULL;
+}

I am wondering what performance gains have you achieved with these skb
pools? Can we see any numbers?

I do not think the control path performance is worth the complexity of
the multilayer skb allocation. In the data packet Rx path, you need to
allocate skb anyway as soon as the driver passes them to the stack. So
what is the gain?

[skipped]

Agree, we are removing the skb pools for the control path.

Regarding Rx data path, we'll get some numbers to see if the pool is 
worth it,

otherwise remove it too.

[skipped]