Date:   Tue, 21 Mar 2023 10:05:22 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        pabeni@...hat.com, Bagas Sanjaya <bagasdotme@...il.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        corbet@....net, jesse.brandeburg@...el.com,
        anthony.l.nguyen@...el.com, pisa@....felk.cvut.cz,
        mkl@...gutronix.de, linux-doc@...r.kernel.org, f.fainelli@...il.com
Subject: Re: [PATCH net-next v2] docs: networking: document NAPI

On Mon, 20 Mar 2023 22:03:34 -0700
Jakub Kicinski <kuba@...nel.org> wrote:

> Add basic documentation about NAPI. We can stop linking to the ancient
> doc on the LF wiki.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> Link: https://lore.kernel.org/all/20230315223044.471002-1-kuba@kernel.org/
> Reviewed-by: Bagas Sanjaya <bagasdotme@...il.com>
> Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---

Looks good overall. I used a grammar scanner to look for issues and
it found lots of little things.

Here are my suggested changes:

diff --git a/Documentation/networking/napi.rst b/Documentation/networking/napi.rst
index e9833f2b777a..822d0bf399af 100644
--- a/Documentation/networking/napi.rst
+++ b/Documentation/networking/napi.rst
@@ -5,19 +5,20 @@ NAPI
 ====
 
 NAPI is the event handling mechanism used by the Linux networking stack.
-The name NAPI does not stand for anything in particular [#]_.
+The name NAPI no longer stands for anything in particular [#]_.
 
-In basic operation device notifies the host about new events via an interrupt.
-The host then schedules a NAPI instance to process the events.
-Device may also be polled for events via NAPI without receiving
+The basic concept of NAPI is that the device notifies
+the kernel about new events via interrupts; then
+the kernel then schedules a NAPI instance to process the events.
+The device may also be polled for events via NAPI without receiving
 interrupts first (:ref:`busy polling<poll>`).
 
 NAPI processing usually happens in the software interrupt context,
-but user may choose to use :ref:`separate kernel threads<threaded>`
+but there is an option to use :ref:`separate kernel threads<threaded>`
 for NAPI processing.
 
-All in all NAPI abstracts away from the drivers the context and configuration
-of event (packet Rx and Tx) processing.
+The goal of NAPI is to abstract the context and configuration of
+event handling.
 
 Driver API
 ==========
@@ -25,7 +26,7 @@ Driver API
 The two most important elements of NAPI are the struct napi_struct
 and the associated poll method. struct napi_struct holds the state
 of the NAPI instance while the method is the driver-specific event
-handler. The method will typically free Tx packets which had been
+handler. The method will typically free Tx packets that have been
 transmitted and process newly received packets.
 
 .. _drv_ctrl:
@@ -44,8 +45,8 @@ to not be invoked. napi_disable() waits for ownership of the NAPI
 instance to be released.
 
 The control APIs are not idempotent. Control API calls are safe against
-concurrent use of datapath APIs but incorrect sequence of control API
-calls may result in crashes, deadlocks, or race conditions. For example
+concurrent use of datapath APIs but an incorrect sequence of control API
+calls may result in crashes, deadlocks, or race conditions. For example,
 calling napi_disable() multiple times in a row will deadlock.
 
 Datapath API
@@ -53,28 +54,30 @@ Datapath API
 
 napi_schedule() is the basic method of scheduling a NAPI poll.
 Drivers should call this function in their interrupt handler
-(see :ref:`drv_sched` for more info). Successful call to napi_schedule()
+(see :ref:`drv_sched` for more info). A successful call to napi_schedule()
 will take ownership of the NAPI instance.
 
-Some time after NAPI is scheduled driver's poll method will be
+Later, after NAPI is scheduled, the driver's poll method will be
 called to process the events/packets. The method takes a ``budget``
 argument - drivers can process completions for any number of Tx
-packets but should only process up to ``budget`` number of
-Rx packets. Rx processing is usually much more expensive.
+packets but should only process up to the ``budget`` number of
+Rx packets. This is ``budget`` argument is used to limit the
+time spent in the poll method when a server is under heavy network
+load.
 
-In other words, it is recommended to ignore the budget argument when
+For most drivers, it is recommended to ignore the budget argument when
 performing TX buffer reclamation to ensure that the reclamation is not
-arbitrarily bounded, however, it is required to honor the budget argument
+arbitrarily bounded; the budget should be honored
 for RX processing.
 
-.. warning::
+.. note::
 
-   ``budget`` may be 0 if core tries to only process Tx completions
+   The ``budget`` argument may be 0 if core tries to only process Tx completions
    and no Rx packets.
 
-The poll method returns amount of work done. If the driver still
+The poll method returns the amount of work done. If the driver still
 has outstanding work to do (e.g. ``budget`` was exhausted)
-the poll method should return exactly ``budget``. In that case
+the poll method should return exactly ``budget``. In that case,
 the NAPI instance will be serviced/polled again (without the
 need to be scheduled).
 
@@ -83,22 +86,21 @@ processed) the poll method should call napi_complete_done()
 before returning. napi_complete_done() releases the ownership
 of the instance.
 
-.. warning::
+.. note::
 
-   The case of finishing all events and using exactly ``budget``
-   must be handled carefully. There is no way to report this
-   (rare) condition to the stack, so the driver must either
-   not call napi_complete_done() and wait to be called again,
+   The case of finishing all events and processing the full ``budget``
+   of packets requires special consideration. The driver must
+   either call napi_complete_done() (and wait to be called again)
    or return ``budget - 1``.
 
-   If ``budget`` is 0 napi_complete_done() should never be called.
+   If the ``budget`` is 0 napi_complete_done() should never be called.
 
 Call sequence
 -------------
 
 Drivers should not make assumptions about the exact sequencing
-of calls. The poll method may be called without driver scheduling
-the instance (unless the instance is disabled). Similarly
+of calls. The poll method may be called without the driver scheduling
+the instance (unless the instance is disabled). Similarly,
 it's not guaranteed that the poll method will be called, even
 if napi_schedule() succeeded (e.g. if the instance gets disabled).
 
@@ -129,7 +131,7 @@ and __napi_schedule() calls:
       __napi_schedule(&v->napi);
   }
 
-IRQ should only be unmasked after successful call to napi_complete_done():
+IRQ should only be unmasked after a successful call to napi_complete_done():
 
 .. code-block:: c
 
@@ -150,7 +152,7 @@ Instance to queue mapping
 Modern devices have multiple NAPI instances (struct napi_struct) per
 interface. There is no strong requirement on how the instances are
 mapped to queues and interrupts. NAPI is primarily a polling/processing
-abstraction without many user-facing semantics. That said, most networking
+abstraction without specific user-facing semantics. That said, most networking
 devices end up using NAPI in fairly similar ways.
 
 NAPI instances most often correspond 1:1:1 to interrupts and queue pairs
@@ -164,7 +166,7 @@ a 1:1 mapping between NAPI instances and interrupts.
 It's worth noting that the ethtool API uses a "channel" terminology where
 each channel can be either ``rx``, ``tx`` or ``combined``. It's not clear
 what constitutes a channel, the recommended interpretation is to understand
-a channel as an IRQ/NAPI which services queues of a given type. For example
+a channel as an IRQ/NAPI which services queues of a given type. For example,
 a configuration of 1 ``rx``, 1 ``tx`` and 1 ``combined`` channel is expected
 to utilize 3 interrupts, 2 Rx and 2 Tx queues.
 
@@ -194,12 +196,12 @@ before NAPI gives up and goes back to using hardware IRQs.
 Busy polling
 ------------
 
-Busy polling allows user process to check for incoming packets before
-the device interrupt fires. As is the case with any busy polling it trades
-off CPU cycles for lower latency (in fact production uses of NAPI busy
+Busy polling allows a user process to check for incoming packets before
+the device interrupt fires. This mode trades off CPU cycles
+for lower latency (in fact production uses of NAPI busy
 polling are not well known).
 
-User can enable busy polling by either setting ``SO_BUSY_POLL`` on
+Busy polling is enabled by either setting ``SO_BUSY_POLL`` on
 selected sockets or using the global ``net.core.busy_poll`` and
 ``net.core.busy_read`` sysctls. An io_uring API for NAPI busy polling
 also exists.
@@ -218,7 +220,7 @@ of packets.
 Such applications can pledge to the kernel that they will perform a busy
 polling operation periodically, and the driver should keep the device IRQs
 permanently masked. This mode is enabled by using the ``SO_PREFER_BUSY_POLL``
-socket option. To avoid the system misbehavior the pledge is revoked
+socket option. To avoid system misbehavior the pledge is revoked
 if ``gro_flush_timeout`` passes without any busy poll call.
 
 The NAPI budget for busy polling is lower than the default (which makes
@@ -231,7 +233,7 @@ with the ``SO_BUSY_POLL_BUDGET`` socket option.
 Threaded NAPI
 -------------
 
-Threaded NAPI is an operating mode which uses dedicated kernel
+Threaded NAPI is an operating mode that uses dedicated kernel
 threads rather than software IRQ context for NAPI processing.
 The configuration is per netdevice and will affect all
 NAPI instances of that device. Each NAPI instance will spawn a separate

Powered by blists - more mailing lists