On Wed, Apr 26, 2023 at 08:19:10PM +0200, Alexander Bluhm wrote:
>
> On Sat, Apr 15, 2023 at 10:44:15PM +0800, Kevin Lo wrote:
> > On Fri, Apr 14, 2023 at 02:01:29PM +0200, Alexander Bluhm wrote:
> > > I think you are trying to change the kernel in the wrong direction.
> > > It should not fail, but handle the requests. Panic if there is a
> > > bug.
> > >
> > > Why do you think M_CANFAIL is a good thing at this place?
> >
> > Because M_WAITOK will not return NULl, I think adding M_CANFAIL will
> > keep the mallocarray call unchanged.
>
> The goal is not to handle errors by failing, but to prevent them.
> Better keep M_WAITOK, avoid M_CANFAIL, and remove the check.
>
> Discussion in the hackroom concluded that M_CANFAIL is for input
> from userland that can be unlimited large. If userland requests
> too much, it can fail. But it is not for normal operation of the
> kernel.
>
> M_NOWAIT should be used when the kernel has a good way to deal with
> a temporary failure, e.g. drop the packet.
>
> M_CANFAIL | M_WAITOK deals with user input that cannot be fullfilled
> permanently and should be reported as an error.
>
> M_WAITOK is for all other cases. If it panics, fix the underlying
> bug that requested unrealistic memory size.
>
> Here use number of queues which should be reasonable low and limited
> by the driver code. Keep M_WAITOK and remove the error check.
Thank you for discussing this topic at the hackathon, and I also greatly
appreciate your detailed explanation.
> By the way, M_DEVBUF is also wrong. It should be M_TEMP as it is
> freed in the same function and not stored permanently. But I wont
> fix that now as malloc(9) type usage is far from consistent.
>
> ok?
ok kevlo@
> bluhm
>
> Index: dev/pci/if_igc.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_igc.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 if_igc.c
> --- dev/pci/if_igc.c 9 Mar 2023 00:13:47 -0000 1.12
> +++ dev/pci/if_igc.c 21 Apr 2023 18:25:35 -0000
> @@ -1209,9 +1209,8 @@ igc_rxrinfo(struct igc_softc *sc, struct
> struct rx_ring *rxr;
> int error, i, n = 0;
>
> - if ((ifr = mallocarray(sc->sc_nqueues, sizeof(*ifr), M_DEVBUF,
> - M_WAITOK | M_ZERO)) == NULL)
> - return ENOMEM;
> + ifr = mallocarray(sc->sc_nqueues, sizeof(*ifr), M_DEVBUF,
> + M_WAITOK | M_ZERO);
>
> for (i = 0; i < sc->sc_nqueues; i++) {
> rxr = &sc->rx_rings[i];
> Index: dev/pci/if_ix.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_ix.c,v
> retrieving revision 1.192
> diff -u -p -r1.192 if_ix.c
> --- dev/pci/if_ix.c 6 Feb 2023 20:27:44 -0000 1.192
> +++ dev/pci/if_ix.c 21 Apr 2023 18:25:35 -0000
> @@ -640,9 +640,8 @@ ixgbe_rxrinfo(struct ix_softc *sc, struc
> u_int n = 0;
>
> if (sc->num_queues > 1) {
> - if ((ifr = mallocarray(sc->num_queues, sizeof(*ifr), M_DEVBUF,
> - M_WAITOK | M_ZERO)) == NULL)
> - return (ENOMEM);
> + ifr = mallocarray(sc->num_queues, sizeof(*ifr), M_DEVBUF,
> + M_WAITOK | M_ZERO);
> } else
> ifr = &ifr1;
>
> Index: dev/pci/if_oce.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_oce.c,v
> retrieving revision 1.106
> diff -u -p -r1.106 if_oce.c
> --- dev/pci/if_oce.c 11 Mar 2022 18:00:48 -0000 1.106
> +++ dev/pci/if_oce.c 21 Apr 2023 18:25:35 -0000
> @@ -902,9 +902,8 @@ oce_rxrinfo(struct oce_softc *sc, struct
> u_int n = 0;
>
> if (sc->sc_nrq > 1) {
> - if ((ifr = mallocarray(sc->sc_nrq, sizeof(*ifr), M_DEVBUF,
> - M_WAITOK | M_ZERO)) == NULL)
> - return (ENOMEM);
> + ifr = mallocarray(sc->sc_nrq, sizeof(*ifr), M_DEVBUF,
> + M_WAITOK | M_ZERO);
> } else
> ifr = &ifr1;
>
>
