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

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

Details

Reviewers

vangyzen
markj
np

Commits

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

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cem created this revision.Sep 21 2018, 5:06 PM

Harbormaster completed remote builds in B19732: Diff 48322.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.

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.

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.