git: 3d65fd97e85a - main - netmap: iflib: enable/disable krings on any interface reinit

[Cy Schubert]

In message <202101101205.10AC5ZrS084658 at gitrepo.freebsd.org>, Vincenzo 
Maffione
 writes:
> The branch main has been updated by vmaffione:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=3d65fd97e85ab807f3baa62aa979ae52
> 7914a3ea
>
> commit 3d65fd97e85ab807f3baa62aa979ae527914a3ea
> Author:     Vincenzo Maffione <vmaffione at FreeBSD.org>
> AuthorDate: 2021-01-10 12:00:30 +0000
> Commit:     Vincenzo Maffione <vmaffione at FreeBSD.org>
> CommitDate: 2021-01-10 12:04:08 +0000
>
>     netmap: iflib: enable/disable krings on any interface reinit
>     
>     Since 1d238b07d5d4d9660ae0e0, krings are disabled before
>     a reinit cycle triggered by iflib_netmap_register.
>     However, this operation is actually necessary also for
>     any interface reinit triggered by other causes (i.e.,
>     ifconfig commands).
>     We achieve this goal by moving the krings enable/disable
>     operation inside iflib_stop() and iflib_init_locked().
>     
>     Once here, this change also removes some redundant operations
>     from iflib_netmap_register(), that are already performed by
>     iflib_stop().
>     
>     PR:             252453
>     MFC after:      1 week
> ---
>  sys/net/iflib.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/sys/net/iflib.c b/sys/net/iflib.c
> index cf02b1574c10..3de80ecaeb0c 100644
> --- a/sys/net/iflib.c
> +++ b/sys/net/iflib.c
> @@ -801,27 +801,18 @@ iflib_netmap_register(struct netmap_adapter *na, int on
> off)
>  	int status;
>  
>  	CTX_LOCK(ctx);
> -	IFDI_INTR_DISABLE(ctx);
> -
> -	/* Tell the stack that the interface is no longer active */
> -	ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE);
> -
>  	if (!CTX_IS_VF(ctx))
>  		IFDI_CRCSTRIP_SET(ctx, onoff, iflib_crcstrip);
>  
> -	/*
> -	 * Stop any pending txsync/rxsync and prevent new ones
> -	 * form starting. Processes blocked in poll() will get
> -	 * POLLERR.
> -	 */
> -	netmap_disable_all_rings(ifp);
> -
>  	iflib_stop(ctx);
>  
>  	/*
>  	 * Enable (or disable) netmap flags, and intercept (or restore)
>  	 * ifp->if_transmit. This is done once the device has been stopped
> -	 * to prevent race conditions.
> +	 * to prevent race conditions. Also, this must be done after
> +	 * calling netmap_disable_all_rings() and before calling
> +	 * netmap_enable_all_rings(), so that these two functions see the
> +	 * updated state of the NAF_NETMAP_ON bit.
>  	 */
>  	if (onoff) {
>  		nm_set_native_flags(na);
> @@ -835,8 +826,6 @@ iflib_netmap_register(struct netmap_adapter *na, int onof
> f)
>  	if (status)
>  		nm_clear_native_flags(na);
>  	CTX_UNLOCK(ctx);
> -        /* Re-enable txsync/rxsync. */
> -	netmap_enable_all_rings(ifp);
>  	return (status);
>  }
>  
> @@ -2388,6 +2377,12 @@ iflib_init_locked(if_ctx_t ctx)
>  	if_setdrvflagbits(ifp, IFF_DRV_OACTIVE, IFF_DRV_RUNNING);
>  	IFDI_INTR_DISABLE(ctx);
>  
> +	/*
> +	 * See iflib_stop(). Useful in case iflib_init_locked() is
> +	 * called without first calling iflib_stop().
> +	 */
> +	netmap_disable_all_rings(ifp);
> +
>  	tx_ip_csum_flags = scctx->isc_tx_csum_flags & (CSUM_IP | CSUM_TCP | CSU
> M_UDP | CSUM_SCTP);
>  	tx_ip6_csum_flags = scctx->isc_tx_csum_flags & (CSUM_IP6_TCP | CSUM_IP6
> _UDP | CSUM_IP6_SCTP);
>  	/* Set hardware offload abilities */
> @@ -2444,6 +2439,9 @@ done:
>  	for (i = 0; i < sctx->isc_ntxqsets; i++, txq++)
>  		callout_reset_on(&txq->ift_timer, iflib_timer_default, iflib_ti
> mer, txq,
>  			txq->ift_timer.c_cpu);
> +
> +        /* Re-enable txsync/rxsync. */
> +	netmap_enable_all_rings(ifp);
>  }
>  
>  static int
> @@ -2489,6 +2487,13 @@ iflib_stop(if_ctx_t ctx)
>  	IFDI_STOP(ctx);
>  	DELAY(1000);
>  
> +	/*
> +	 * Stop any pending txsync/rxsync and prevent new ones
> +	 * form starting. Processes blocked in poll() will get
> +	 * POLLERR.
> +	 */
> +	netmap_disable_all_rings(ctx->ifc_ifp);
> +
>  	iflib_debug_reset();
>  	/* Wait for current tx queue users to exit to disarm watchdog timer. */
>  	for (i = 0; i < scctx->isc_ntxqsets; i++, txq++) {
>

This commit makes a couple of assumptions

1. It's missing definition in a header for netmap_disable_all_rings() and 
netmap_enable_all_rings().

2. It assumes that all kernels include device, that it is not loaded as a 
kld.

The first assumption causes a build failure.

The second assumption might require at the very least an UPDATING entry to 
warn people not to remove netmap from their kernels or rely on the module. 
And probably needs a __FreeBSD_version bump.


-- 
Cheers,
Cy Schubert <Cy.Schubert at cschubert.com>
FreeBSD UNIX:  <cy at FreeBSD.org>   Web:  https://FreeBSD.org
NTP:           <cy at nwtime.org>    Web:  https://nwtime.org

	The need of the many outweighs the greed of the few.