From: Markus Schneider-Pargmann <msp@baylibre.com>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>,
Chandrasekar Ramakrishnan <rcsekar@samsung.com>,
Wolfgang Grandegger <wg@grandegger.com>,
linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 18/18] can: m_can: Implement transmit submission coalescing
Date: Mon, 30 Jan 2023 09:05:41 +0100 [thread overview]
Message-ID: <20230130080541.b7lnbc44qrfylyyx@blmsp> (raw)
In-Reply-To: <CAMZ6RqJrCxWTkY1tZKXcXTKSJyQFS9kqgL_JwxL6j99ysY+JgA@mail.gmail.com>
Hi Vincent,
On Sun, Jan 29, 2023 at 12:00:48AM +0900, Vincent MAILHOL wrote:
> Hi Markus,
>
> I did not have time for a deep review yet. But so far, I didn't find
> anything odd aside from two nitpicks.
>
> On Thu. 26 Jan 2023 à 04:53, Markus Schneider-Pargmann
> <msp@baylibre.com> a écrit :
> >
> > m_can supports submitting mulitple transmits with one register write.
> ^^^^^^^^
> multiple
>
> > This is an interesting option to reduce the number of SPI transfers for
> > peripheral chips.
> >
> > The m_can_tx_op is extended with a bool that signals if it is the last
> > transmission and the submit should be executed immediately.
> >
> > The worker then writes the skb to the FIFO and submits it only if the
> > submit bool is set. If it isn't set, the worker will write the next skb
> > which is waiting in the workqueue to the FIFO, etc.
> >
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> > ---
> >
> > Notes:
> > Notes:
> > - I ran into lost messages in the receive FIFO when using this
> > implementation. I guess this only shows up with my test setup in
> > loopback mode and maybe not enough CPU power.
> > - I put this behind the tx-frames ethtool coalescing option as we do
> > wait before submitting packages but it is something different than the
> > tx-frames-irq option. I am not sure if this is the correct option,
> > please let me know.
> >
> > drivers/net/can/m_can/m_can.c | 55 ++++++++++++++++++++++++++++++++---
> > drivers/net/can/m_can/m_can.h | 6 ++++
> > 2 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index c6a09369d1aa..99bfcfec3775 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1504,6 +1504,9 @@ static int m_can_start(struct net_device *dev)
> > if (ret)
> > return ret;
> >
> > + netdev_queue_set_dql_min_limit(netdev_get_tx_queue(cdev->net, 0),
> > + cdev->tx_max_coalesced_frames);
> > +
> > cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> >
> > m_can_enable_all_interrupts(cdev);
> > @@ -1813,8 +1816,13 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
> > */
> > can_put_echo_skb(skb, dev, putidx, frame_len);
> >
> > - /* Enable TX FIFO element to start transfer */
> > - m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
> > + if (cdev->is_peripheral) {
> > + /* Delay enabling TX FIFO element */
> > + cdev->tx_peripheral_submit |= BIT(putidx);
> > + } else {
> > + /* Enable TX FIFO element to start transfer */
> > + m_can_write(cdev, M_CAN_TXBAR, BIT(putidx));
> > + }
> > cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
> > 0 : cdev->tx_fifo_putidx);
> > }
> > @@ -1827,6 +1835,17 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
> > return NETDEV_TX_BUSY;
> > }
> >
> > +static void m_can_tx_submit(struct m_can_classdev *cdev)
> > +{
> > + if (cdev->version == 30)
> > + return;
> > + if (!cdev->is_peripheral)
> > + return;
> > +
> > + m_can_write(cdev, M_CAN_TXBAR, cdev->tx_peripheral_submit);
> > + cdev->tx_peripheral_submit = 0;
> > +}
> > +
> > static void m_can_tx_work_queue(struct work_struct *ws)
> > {
> > struct m_can_tx_op *op = container_of(ws, struct m_can_tx_op, work);
> > @@ -1835,11 +1854,15 @@ static void m_can_tx_work_queue(struct work_struct *ws)
> >
> > op->skb = NULL;
> > m_can_tx_handler(cdev, skb);
> > + if (op->submit)
> > + m_can_tx_submit(cdev);
> > }
> >
> > -static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb)
> > +static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb,
> > + bool submit)
> > {
> > cdev->tx_ops[cdev->next_tx_op].skb = skb;
> > + cdev->tx_ops[cdev->next_tx_op].submit = submit;
> > queue_work(cdev->tx_wq, &cdev->tx_ops[cdev->next_tx_op].work);
> >
> > ++cdev->next_tx_op;
> > @@ -1851,6 +1874,7 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
> > struct sk_buff *skb)
> > {
> > netdev_tx_t err;
> > + bool submit;
> >
> > if (cdev->can.state == CAN_STATE_BUS_OFF) {
> > m_can_clean(cdev->net);
> > @@ -1861,7 +1885,15 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
> > if (err != NETDEV_TX_OK)
> > return err;
> >
> > - m_can_tx_queue_skb(cdev, skb);
> > + ++cdev->nr_txs_without_submit;
> > + if (cdev->nr_txs_without_submit >= cdev->tx_max_coalesced_frames ||
> > + !netdev_xmit_more()) {
> > + cdev->nr_txs_without_submit = 0;
> > + submit = true;
> > + } else {
> > + submit = false;
> > + }
> > + m_can_tx_queue_skb(cdev, skb, submit);
> >
> > return NETDEV_TX_OK;
> > }
> > @@ -1993,6 +2025,7 @@ static int m_can_get_coalesce(struct net_device *dev,
> >
> > ec->rx_max_coalesced_frames_irq = cdev->rx_max_coalesced_frames_irq;
> > ec->rx_coalesce_usecs_irq = cdev->rx_coalesce_usecs_irq;
> > + ec->tx_max_coalesced_frames = cdev->tx_max_coalesced_frames;
> > ec->tx_max_coalesced_frames_irq = cdev->tx_max_coalesced_frames_irq;
> > ec->tx_coalesce_usecs_irq = cdev->tx_coalesce_usecs_irq;
> >
> > @@ -2037,6 +2070,18 @@ static int m_can_set_coalesce(struct net_device *dev,
> > netdev_err(dev, "tx-frames-irq and tx-usecs-irq can only be set together\n");
> > return -EINVAL;
> > }
> > + if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXE].num) {
> > + netdev_err(dev, "tx-frames (%u) greater than the TX event FIFO (%u)\n",
> ^^^^
> ^^^^
> Avoid parenthesis.
> Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages:
>
> Printing numbers in parentheses (%d) adds no value and should be avoided.
>
> This comment applies to other patches in the series as well.
Thank you for having a look and pointing this out, I wasn't aware of
that. I will fix these for the next version.
Best,
Markus
>
> > + ec->tx_max_coalesced_frames,
> > + cdev->mcfg[MRAM_TXE].num);
> > + return -EINVAL;
> > + }
> > + if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXB].num) {
> > + netdev_err(dev, "tx-frames (%u) greater than the TX FIFO (%u)\n",
> > + ec->tx_max_coalesced_frames,
> > + cdev->mcfg[MRAM_TXB].num);
> > + return -EINVAL;
> > + }
> > if (ec->rx_coalesce_usecs_irq != 0 && ec->tx_coalesce_usecs_irq != 0 &&
> > ec->rx_coalesce_usecs_irq != ec->tx_coalesce_usecs_irq) {
> > netdev_err(dev, "rx-usecs-irq (%u) needs to be equal to tx-usecs-irq (%u) if both are enabled\n",
> > @@ -2047,6 +2092,7 @@ static int m_can_set_coalesce(struct net_device *dev,
> >
> > cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
> > cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
> > + cdev->tx_max_coalesced_frames = ec->tx_max_coalesced_frames;
> > cdev->tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
> > cdev->tx_coalesce_usecs_irq = ec->tx_coalesce_usecs_irq;
> >
> > @@ -2064,6 +2110,7 @@ static const struct ethtool_ops m_can_ethtool_ops = {
> > .supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
> > ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |
> > ETHTOOL_COALESCE_TX_USECS_IRQ |
> > + ETHTOOL_COALESCE_TX_MAX_FRAMES |
> > ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
> > .get_ts_info = ethtool_op_get_ts_info,
> > .get_coalesce = m_can_get_coalesce,
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index bfef2c89e239..e209de81b5a4 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -74,6 +74,7 @@ struct m_can_tx_op {
> > struct m_can_classdev *cdev;
> > struct work_struct work;
> > struct sk_buff *skb;
> > + bool submit;
> > };
> >
> > struct m_can_classdev {
> > @@ -103,6 +104,7 @@ struct m_can_classdev {
> > u32 active_interrupts;
> > u32 rx_max_coalesced_frames_irq;
> > u32 rx_coalesce_usecs_irq;
> > + u32 tx_max_coalesced_frames;
> > u32 tx_max_coalesced_frames_irq;
> > u32 tx_coalesce_usecs_irq;
> >
> > @@ -117,6 +119,10 @@ struct m_can_classdev {
> > int tx_fifo_size;
> > int next_tx_op;
> >
> > + int nr_txs_without_submit;
> > + /* bitfield of fifo elements that will be submitted together */
> > + u32 tx_peripheral_submit;
> > +
> > struct mram_cfg mcfg[MRAM_CFG_NUM];
> > };
prev parent reply other threads:[~2023-01-30 8:05 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-25 19:50 [PATCH v2 00/18] can: m_can: Optimizations for m_can/tcan part 2 Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 01/18] can: tcan4x5x: Remove reserved register 0x814 from writable table Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 02/18] can: tcan4x5x: Check size of mram configuration Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 03/18] can: m_can: Remove repeated check for is_peripheral Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 04/18] can: m_can: Always acknowledge all interrupts Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 05/18] can: m_can: Remove double interrupt enable Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 06/18] can: m_can: Disable unused interrupts Markus Schneider-Pargmann
2023-01-26 8:07 ` Simon Horman
2023-01-25 19:50 ` [PATCH v2 07/18] can: m_can: Keep interrupts enabled during peripheral read Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 08/18] can: m_can: Write transmit header and data in one transaction Markus Schneider-Pargmann
2023-01-26 8:04 ` Simon Horman
2023-01-30 8:04 ` Markus Schneider-Pargmann
2023-02-04 13:05 ` Simon Horman
2023-02-20 5:31 ` Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 09/18] can: m_can: Implement receive coalescing Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 10/18] can: m_can: Implement transmit coalescing Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 11/18] can: m_can: Add rx coalescing ethtool support Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 12/18] can: m_can: Add tx " Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 13/18] can: m_can: Cache tx putidx Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 14/18] can: m_can: Use the workqueue as queue Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 15/18] can: m_can: Introduce a tx_fifo_in_flight counter Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 16/18] can: m_can: Use tx_fifo_in_flight for netif_queue control Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 17/18] can: m_can: Implement BQL Markus Schneider-Pargmann
2023-01-25 19:50 ` [PATCH v2 18/18] can: m_can: Implement transmit submission coalescing Markus Schneider-Pargmann
2023-01-28 15:00 ` Vincent MAILHOL
2023-01-30 8:05 ` Markus Schneider-Pargmann [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230130080541.b7lnbc44qrfylyyx@blmsp \
--to=msp@baylibre.com \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=rcsekar@samsung.com \
--cc=wg@grandegger.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).