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

[Vincenzo Maffione]

Il giorno dom 10 gen 2021 alle ore 15:33 Cy Schubert <
Cy.Schubert at cschubert.com> ha scritto:

> 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++) {
> >
>
>
Sorry, that was not intentional.


> 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().
>

The header definitions are actually in sys/dev/netmap/netmap_kern.h


>
> 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.
>

I pushed another commit to (hopefully) fix the issue. I don't think any of
this is necessary, because iflib already
supports the case where DEV_NETMAP is not defined. I simply forgot to add a
placeholder.

Thanks


> --
> 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.
>
>
>