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

From: Loic Poulain
Date: Wed Nov 23 2022 - 16:37:08 EST

Next message: Andrii Nakryiko: "Re: [PATCH] samples, bpf: Add duration option D for sampleip"
Previous message: Yosry Ahmed: "Re: [RFC PATCH V1] mm: Disable demotion from proactive reclaim"
In reply to: haozhe.chang: "[PATCH v4] wwan: core: Support slicing in port TX flow of WWAN subsystem"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 22 Nov 2022 at 09:25, <haozhe.chang@xxxxxxxxxxxx> wrote:
>
> From: haozhe chang <haozhe.chang@xxxxxxxxxxxx>
>
> 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 frag_len) or not. By
> doing so, the additional memory copy is reduced.
>
> Meanwhile, this patch gives WWAN devices driver the option to reserve
> headroom in fragments for the device-specific metadata.
>
> Signed-off-by: haozhe chang <haozhe.chang@xxxxxxxxxxxx>
>
> ---
> Changes in v2
> -send fragments to device driver by skb frag_list.
>
> Changes in v3
> -move frag_len and headroom_len setting to wwan_create_port.
>
> Changes in v4
> -change unreadable parameters to macro definition.
> ---
> drivers/net/wwan/iosm/iosm_ipc_port.c | 3 +-
> drivers/net/wwan/mhi_wwan_ctrl.c | 3 +-
> drivers/net/wwan/rpmsg_wwan_ctrl.c | 3 +-
> drivers/net/wwan/t7xx/t7xx_port_wwan.c | 34 +++++++--------
> drivers/net/wwan/wwan_core.c | 59 ++++++++++++++++++++------
> drivers/net/wwan/wwan_hwsim.c | 1 +
> drivers/usb/class/cdc-wdm.c | 3 +-
> include/linux/wwan.h | 15 +++++++
> 8 files changed, 86 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/wwan/iosm/iosm_ipc_port.c b/drivers/net/wwan/iosm/iosm_ipc_port.c
> index b6d81c627277..7798348f61d0 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_port.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_port.c
> @@ -63,7 +63,8 @@ struct iosm_cdev *ipc_port_init(struct iosm_imem *ipc_imem,
> ipc_port->ipc_imem = ipc_imem;
>
> ipc_port->iosm_port = wwan_create_port(ipc_port->dev, port_type,
> - &ipc_wwan_ctrl_ops, ipc_port);
> + &ipc_wwan_ctrl_ops, WWAN_NO_FRAGMENT,
> + WWAN_NO_HEADROOM, ipc_port);
>
> return ipc_port;
> }
> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index f7ca52353f40..c397aa53db5d 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -237,7 +237,8 @@ static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
>
> /* Register as a wwan port, id->driver_data contains wwan port type */
> port = wwan_create_port(&cntrl->mhi_dev->dev, id->driver_data,
> - &wwan_pops, mhiwwan);
> + &wwan_pops, WWAN_NO_FRAGMENT, WWAN_NO_HEADROOM,
> + mhiwwan);
> if (IS_ERR(port)) {
> kfree(mhiwwan);
> return PTR_ERR(port);
> diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> index 31c24420ab2e..fc6c228b7e1c 100644
> --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
> +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> @@ -129,7 +129,8 @@ static int rpmsg_wwan_ctrl_probe(struct rpmsg_device *rpdev)
>
> /* Register as a wwan port, id.driver_data contains wwan port type */
> port = wwan_create_port(parent, rpdev->id.driver_data,
> - &rpmsg_wwan_pops, rpwwan);
> + &rpmsg_wwan_pops, WWAN_NO_FRAGMENT,
> + WWAN_NO_HEADROOM, rpwwan);
> if (IS_ERR(port))
> return PTR_ERR(port);
>
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> index 33931bfd78fd..b75bb272f861 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 int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> {
> struct t7xx_port *port_private = wwan_port_get_drvdata(port);
> - size_t len, offset, chunk_len = 0, txq_mtu = CLDMA_MTU;
> const struct t7xx_port_conf *port_conf;
> + struct sk_buff *cur = skb, *cloned;
> struct t7xx_fsm_ctl *ctl;
> enum md_state md_state;
> + int cnt = 0, ret;
>
> - len = skb->len;
> - if (!len || !port_private->chan_enable)
> + if (!port_private->chan_enable)
> return -EINVAL;
>
> port_conf = port_private->port_conf;
> @@ -72,23 +72,21 @@ static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> return -ENODEV;
> }
>
> - for (offset = 0; offset < len; offset += chunk_len) {
> - struct sk_buff *skb_ccci;
> - int ret;
> -
> - chunk_len = min(len - offset, txq_mtu - sizeof(struct ccci_header));
> - skb_ccci = t7xx_port_alloc_skb(chunk_len);
> - if (!skb_ccci)
> - return -ENOMEM;
> -
> - skb_put_data(skb_ccci, skb->data + offset, chunk_len);
> - ret = t7xx_port_send_skb(port_private, skb_ccci, 0, 0);
> + while (cur) {
> + cloned = skb_clone(cur, GFP_KERNEL);
> + cloned->len = skb_headlen(cur);
> + ret = t7xx_port_send_skb(port_private, cloned, 0, 0);
> if (ret) {
> - dev_kfree_skb_any(skb_ccci);
> + dev_kfree_skb(cloned);
> dev_err(port_private->dev, "Write error on %s port, %d\n",
> port_conf->name, ret);
> - return ret;
> + return cnt ? cnt + ret : ret;
> }
> + cnt += cur->len;
> + if (cur == skb)
> + cur = skb_shinfo(skb)->frag_list;
> + else
> + cur = cur->next;
> }
>
> dev_kfree_skb(skb);
> @@ -154,13 +152,15 @@ static int t7xx_port_wwan_disable_chl(struct t7xx_port *port)
> static void t7xx_port_wwan_md_state_notify(struct t7xx_port *port, unsigned int state)
> {
> const struct t7xx_port_conf *port_conf = port->port_conf;
> + unsigned int header_len = sizeof(struct ccci_header);
>
> if (state != MD_STATE_READY)
> return;
>
> if (!port->wwan_port) {
> port->wwan_port = wwan_create_port(port->dev, port_conf->port_type,
> - &wwan_ops, port);
> + &wwan_ops, CLDMA_MTU - header_len,
> + header_len, port);
> if (IS_ERR(port->wwan_port))
> dev_err(port->dev, "Unable to create WWWAN port %s", port_conf->name);
> }
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index 62e9f7d6c9fe..8d35513bcd4c 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -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
> + * @frag_len: Length to fragment 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 frag_len;
> union {
> struct {
> struct ktermios termios;
> @@ -422,6 +426,8 @@ static int __wwan_port_dev_assign_name(struct wwan_port *port, const char *fmt)
> struct wwan_port *wwan_create_port(struct device *parent,
> enum wwan_port_type type,
> const struct wwan_port_ops *ops,
> + size_t frag_len,
> + unsigned int headroom_len,
> void *drvdata)
> {
> struct wwan_device *wwandev;
> @@ -455,6 +461,8 @@ struct wwan_port *wwan_create_port(struct device *parent,
>
> port->type = type;
> port->ops = ops;
> + port->frag_len = frag_len ? frag_len : SIZE_MAX;
> + port->headroom_len = headroom_len;
> mutex_init(&port->ops_lock);
> skb_queue_head_init(&port->rxq);
> init_waitqueue_head(&port->waitqueue);
> @@ -698,30 +706,53 @@ 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)
> {
> + struct sk_buff *skb, *head = NULL, *tail = NULL;
> struct wwan_port *port = filp->private_data;
> - struct sk_buff *skb;
> + size_t frag_len, remain = count;
> 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;
> + do {
> + frag_len = min(remain, port->frag_len);
> + skb = alloc_skb(frag_len + port->headroom_len, GFP_KERNEL);
> + 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, frag_len), buf + count - remain, frag_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;
> + }
> + } while (remain -= frag_len);
> +
> + ret = wwan_port_op_tx(port, head, !!(filp->f_flags & O_NONBLOCK));
> + if (!ret)
> + return count;
>
> - return count;
> +freeskb:
> + kfree_skb(head);
> + return ret;
> }
>
> static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
> diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
> index ff09a8cedf93..7fb54cb51628 100644
> --- a/drivers/net/wwan/wwan_hwsim.c
> +++ b/drivers/net/wwan/wwan_hwsim.c
> @@ -205,6 +205,7 @@ static struct wwan_hwsim_port *wwan_hwsim_port_new(struct wwan_hwsim_dev *dev)
>
> port->wwan = wwan_create_port(&dev->dev, WWAN_PORT_AT,
> &wwan_hwsim_port_ops,
> + WWAN_NO_FRAGMENT, WWAN_NO_HEADROOM,
> port);
> if (IS_ERR(port->wwan)) {
> err = PTR_ERR(port->wwan);
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 1f0951be15ab..e0f0bc878bbd 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -929,7 +929,8 @@ static void wdm_wwan_init(struct wdm_device *desc)
> return;
> }
>
> - port = wwan_create_port(&intf->dev, desc->wwanp_type, &wdm_wwan_port_ops, desc);
> + port = wwan_create_port(&intf->dev, desc->wwanp_type, &wdm_wwan_port_ops,
> + WWAN_NO_FRAGMENT, WWAN_NO_HEADROOM, desc);
> if (IS_ERR(port)) {
> dev_err(&intf->dev, "%s: Unable to create WWAN port\n",
> dev_name(intf->usb_dev));
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 5ce2acf444fb..37f25ebb9733 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h
> @@ -62,11 +62,24 @@ struct wwan_port_ops {
> poll_table *wait);
> };
>
> +/*
> + * Used to indicate that the WWAN core should not fragment tx packages.
> + */
> +#define WWAN_NO_FRAGMENT 0
> +
> +/*
> + * Used to indicate that the WWAN core should not reserve headroom in SKB.
> + */
> +#define WWAN_NO_HEADROOM 0

It could be a bit misleading here; these values are only used as
'control ports' parameters, and not for the 'regular' WWAN network
payload. Make this more clear in the above comments/def-names.

Regards,
Loic

> +
> /**
> * wwan_create_port - Add a new WWAN port
> * @parent: Device to use as parent and shared by all WWAN ports
> * @type: WWAN port type
> * @ops: WWAN port operations
> + * @frag_len: TX fragments length, if 0 is set,
> + * the WWAN core don't fragment the user package.
> + * @headroom_len: TX fragments reserved headroom length
> * @drvdata: Pointer to caller driver data
> *
> * Allocate and register a new WWAN port. The port will be automatically exposed
> @@ -84,6 +97,8 @@ struct wwan_port_ops {
> struct wwan_port *wwan_create_port(struct device *parent,
> enum wwan_port_type type,
> const struct wwan_port_ops *ops,
> + size_t frag_len,
> + unsigned int headroom_len,
> void *drvdata);
>
> /**
> --
> 2.17.0
>

Next message: Andrii Nakryiko: "Re: [PATCH] samples, bpf: Add duration option D for sampleip"
Previous message: Yosry Ahmed: "Re: [RFC PATCH V1] mm: Disable demotion from proactive reclaim"
In reply to: haozhe.chang: "[PATCH v4] wwan: core: Support slicing in port TX flow of WWAN subsystem"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]