Page MenuHomeFreeBSD

cxgb(4): Netdump: only reference allocated qsets
ClosedPublic

Authored by cem on Sep 21 2018, 5:06 PM.

Details

Summary

SGE_QSETS is an upper bound -- fewer qsets may be allocated depending on
the number of CPUs.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem created this revision.Sep 21 2018, 5:06 PM
vangyzen accepted this revision.Sep 21 2018, 5:50 PM
This revision is now accepted and ready to land.Sep 21 2018, 5:50 PM
markj accepted this revision.Sep 21 2018, 6:04 PM
markj added inline comments.
sys/dev/cxgb/cxgb_main.c
853 ↗(On Diff #48322)

I'd find it a bit clearer to write sc->nqsets += pi->nqsets at the end of the outer loop, since otherwise you're converting between a quantity and an index. But, up to you.

np added a comment.Sep 21 2018, 6:14 PM

Initializing sc->nqsets here means it may not have a good value until a
port (any port) of the adapter is brought up administratively. This may
work for netdump. If not then setup sc->nqsets in t3_sge_prep instead,
which runs very early.

cem marked an inline comment as done.Sep 21 2018, 10:33 PM
In D17274#368225, @np wrote:

Initializing sc->nqsets here means it may not have a good value until a
port (any port) of the adapter is brought up administratively. This may
work for netdump. If not then setup sc->nqsets in t3_sge_prep instead,
which runs very early.

I think that's ok — my understanding is that netdump needs the interface to already be up and configured at runtime in order to function during panic. The adapter object is allocated by newbus as a softc, which means it has a zero initial value (not undefined). I think that should be ok.

Of course,Mark is more familiar with netdump than I am :-).

sys/dev/cxgb/cxgb_main.c
853 ↗(On Diff #48322)

Quantity is just one greater than the last index for zero-index arrays (in general), and the index is always incremented after use (in this specific case). So at the end of the loop, the value is the quantity. This is common enough in C code that I find it clear.

I'll leave it as-is, unless you feel strongly about it.

In D17274#368307, @cem wrote:
In D17274#368225, @np wrote:

Initializing sc->nqsets here means it may not have a good value until a
port (any port) of the adapter is brought up administratively. This may
work for netdump. If not then setup sc->nqsets in t3_sge_prep instead,
which runs very early.

I think that's ok — my understanding is that netdump needs the interface to already be up and configured at runtime in order to function during panic. The adapter object is allocated by newbus as a softc, which means it has a zero initial value (not undefined). I think that should be ok.

Yeah, it should be safe for netdump routines to assume that the driver is fully initialized.

np accepted this revision.Sep 21 2018, 10:42 PM
This revision was automatically updated to reflect the committed changes.