Re: [PATCH v2] wwan: core: Support slicing in port TX flow of WWAN subsystem

["Haozhe Chang (常浩哲)" <Haozhe.Chang@mediatek.com>]

Hi Loic

On Tue, 2022-11-08 at 13:14 +0100, Loic Poulain wrote:
> Hi Haozhe,
> 
> On Tue, 8 Nov 2022 at 11:54, <haozhe.chang@mediatek.com> wrote:
> > 
> > From: haozhe chang <haozhe.chang@mediatek.com>
> > 
> > wwan_port_fops_write inputs the SKB parameter to the TX callback of
> > the WWAN device driver. However, the WWAN device (e.g., t7xx) may
> > have an MTU less than the size of SKB, causing the TX buffer to be
> > sliced and copied once more in the WWAN device driver.
> > 
> > This patch implements the slicing in the WWAN subsystem and gives
> > the WWAN devices driver the option to slice(by chunk) or not. By
> > doing so, the additional memory copy is reduced.
> > 
> > Meanwhile, this patch gives WWAN devices driver the option to
> > reserve
> > headroom in SKB for the device-specific metadata.
> > 
> > Signed-off-by: haozhe chang <haozhe.chang@mediatek.com>
> > 
> > ---
> > Changes in v2
> >   -send fragments to device driver by skb frag_list.
> > ---
> >  drivers/net/wwan/t7xx/t7xx_port_wwan.c | 42 ++++++++++-------
> >  drivers/net/wwan/wwan_core.c           | 65 ++++++++++++++++++++
> > ------
> >  include/linux/wwan.h                   |  5 +-
> >  3 files changed, 80 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > index 33931bfd78fd..74fa58575d5a 100644
> > --- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > +++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> > @@ -54,13 +54,13 @@ static void t7xx_port_ctrl_stop(struct
> > wwan_port *port)
> 
> [...]
> >  static const struct wwan_port_ops wwan_ops = {
> >         .start = t7xx_port_ctrl_start,
> >         .stop = t7xx_port_ctrl_stop,
> >         .tx = t7xx_port_ctrl_tx,
> > +       .needed_headroom = t7xx_port_tx_headroom,
> > +       .tx_chunk_len = t7xx_port_tx_chunk_len,
> 
> Can you replace 'chunk' with 'frag' everywhere?
> 
OK
> >  };
> > 
> >  static int t7xx_port_wwan_init(struct t7xx_port *port)
> > diff --git a/drivers/net/wwan/wwan_core.c
> > b/drivers/net/wwan/wwan_core.c
> > index 62e9f7d6c9fe..ed78471f9e38 100644
> > --- a/drivers/net/wwan/wwan_core.c
> > +++ b/drivers/net/wwan/wwan_core.c
> > @@ -20,7 +20,7 @@
> >  #include <uapi/linux/wwan.h>
> > 
> >  /* Maximum number of minors in use */
> > -#define WWAN_MAX_MINORS                (1 << MINORBITS)
> > +#define WWAN_MAX_MINORS                BIT(MINORBITS)
> > 
> >  static DEFINE_MUTEX(wwan_register_lock); /* WWAN device
> > create|remove lock */
> >  static DEFINE_IDA(minors); /* minors for WWAN port chardevs */
> > @@ -67,6 +67,8 @@ struct wwan_device {
> >   * @rxq: Buffer inbound queue
> >   * @waitqueue: The waitqueue for port fops (read/write/poll)
> >   * @data_lock: Port specific data access serialization
> > + * @headroom_len: SKB reserved headroom size
> > + * @chunk_len: Chunk len to split packet
> >   * @at_data: AT port specific data
> >   */
> >  struct wwan_port {
> > @@ -79,6 +81,8 @@ struct wwan_port {
> >         struct sk_buff_head rxq;
> >         wait_queue_head_t waitqueue;
> >         struct mutex data_lock; /* Port specific data access
> > serialization */
> > +       size_t headroom_len;
> > +       size_t chunk_len;
> >         union {
> >                 struct {
> >                         struct ktermios termios;
> > @@ -550,8 +554,13 @@ static int wwan_port_op_start(struct wwan_port
> > *port)
> >         }
> > 
> >         /* If port is already started, don't start again */
> > -       if (!port->start_count)
> > +       if (!port->start_count) {
> >                 ret = port->ops->start(port);
> > +               if (port->ops->tx_chunk_len)
> > +                       port->chunk_len = port->ops-
> > >tx_chunk_len(port);
> 
> So, maybe frag len and headroom should be parameters of
> wwan_create_port() instead of port ops, as we really need this info
> only once.
> 
If frag_len and headroom are added, wwan_create_port will have 6
parameters, is it too much? And for similar requirements in the future,
it may be difficult to add more parameters.

Is it a good solution to provide wwan_port_set_frag_len and
wwan_port_set_headroom_len to the device driver? if so, the device
driver has a chance to modify the wwan port's field after calling
wwan_create_port.
> > +               if (port->ops->needed_headroom)
> > +                       port->headroom_len = port->ops-
> > >needed_headroom(port);
> > +       }
> > 
> >         if (!ret)
> >                 port->start_count++;
> > @@ -698,30 +707,56 @@ static ssize_t wwan_port_fops_read(struct
> > file *filp, char __user *buf,
> >  static ssize_t wwan_port_fops_write(struct file *filp, const char
> > __user *buf,
> >                                     size_t count, loff_t *offp)
> >  {
> > +       size_t len, chunk_len, offset, allowed_chunk_len;
> > +       struct sk_buff *skb, *head = NULL, *tail = NULL;
> >         struct wwan_port *port = filp->private_data;
> > -       struct sk_buff *skb;
> >         int ret;
> > 
> >         ret = wwan_wait_tx(port, !!(filp->f_flags & O_NONBLOCK));
> >         if (ret)
> >                 return ret;
> > 
> > -       skb = alloc_skb(count, GFP_KERNEL);
> > -       if (!skb)
> > -               return -ENOMEM;
> > +       allowed_chunk_len = port->chunk_len ? port->chunk_len :
> > count;
> 
> I would suggest making port->chunk_len (frag_len) always valid, by
> setting it to -1 (MAX size_t) when creating a port without frag_len
> requirement.
> 
Ok, it will help to reduce some code.
> > +       for (offset = 0; offset < count; offset += chunk_len) {
> > +               chunk_len = min(count - offset, allowed_chunk_len);
> > +               len = chunk_len + port->headroom_len;
> > +               skb = alloc_skb(len, GFP_KERNEL);
> 
> That works but would prefer a simpler solution like:
> do {
>     len = min(port->frag_len, remain);
>     skb = alloc_skb(len + port->needed_headroom; GFP_KERNEL);
>     [...]
>     copy_from_user(skb_put(skb, len), buf + count - remain)
> } while ((remain -= len));
> 
May I know if the suggestion is because "while" is more efficient
than  "for", or is it more readable?
> > +               if (!skb) {
> > +                       ret = -ENOMEM;
> > +                       goto freeskb;
> > +               }
> > +               skb_reserve(skb, port->headroom_len);
> > +
> > +               if (!head) {
> > +                       head = skb;
> > +               } else if (!tail) {
> > +                       skb_shinfo(head)->frag_list = skb;
> > +                       tail = skb;
> > +               } else {
> > +                       tail->next = skb;
> > +                       tail = skb;
> > +               }
> > 
> > -       if (copy_from_user(skb_put(skb, count), buf, count)) {
> > -               kfree_skb(skb);
> > -               return -EFAULT;
> > -       }
> > +               if (copy_from_user(skb_put(skb, chunk_len), buf +
> > offset, chunk_len)) {
> > +                       ret = -EFAULT;
> > +                       goto freeskb;
> > +               }
> > 
> > -       ret = wwan_port_op_tx(port, skb, !!(filp->f_flags &
> > O_NONBLOCK));
> > -       if (ret) {
> > -               kfree_skb(skb);
> > -               return ret;
> > +               if (skb != head) {
> > +                       head->data_len += skb->len;
> > +                       head->len += skb->len;
> > +                       head->truesize += skb->truesize;
> > +               }
> >         }
> > 
> > -       return count;
> > +       if (head) {
> 
> How head can be null here?
> 
if the parameter "count" is 0, the for loop will not be executed.
> > +               ret = wwan_port_op_tx(port, head, !!(filp->f_flags
> > & O_NONBLOCK));
> > +               if (!ret)
> > +                       return count;
> > +       }
> > +freeskb:
> > +       kfree_skb(head);
> > +       return ret;
> >  }
> > 
> >  static __poll_t wwan_port_fops_poll(struct file *filp, poll_table
> > *wait)
> > diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> > index 5ce2acf444fb..bdeeef59bbfd 100644
> > --- a/include/linux/wwan.h
> > +++ b/include/linux/wwan.h
> > @@ -46,6 +46,8 @@ struct wwan_port;
> >   * @tx: Non-blocking routine that sends WWAN port protocol data to
> > the device.
> >   * @tx_blocking: Optional blocking routine that sends WWAN port
> > protocol data
> >   *               to the device.
> > + * @needed_headroom: Optional routine that sets reserve headroom
> > of skb.
> > + * @tx_chunk_len: Optional routine that sets chunk len to split.
> >   * @tx_poll: Optional routine that sets additional TX poll flags.
> >   *
> >   * The wwan_port_ops structure contains a list of low-level
> > operations
> > @@ -58,6 +60,8 @@ struct wwan_port_ops {
> > 
> >         /* Optional operations */
> >         int (*tx_blocking)(struct wwan_port *port, struct sk_buff
> > *skb);
> > +       size_t (*needed_headroom)(struct wwan_port *port);
> > +       size_t (*tx_chunk_len)(struct wwan_port *port);
> 
> As said above, maybe move that as variables, or parameter of
> wwan_create_port.
> 
> Regards,
> Loic