In D59204#1360536, @netchild wrote:

The patchset is now running on the host which needs the watchdog to trigger from time to time. It needs a while to run into the issue.

In D59294#1360356, @kbowling wrote:

Limit this change to the fe00ca2f8871 regression

Whoops.. this was accidentally committed as part of 6554362c664073f963300598abc4e8ae2be6d915 in 2019. Closing

You never seem to use txr->tx_lock. Eg, you init it, and destroy it, but its not used to serialize access to the txr. And you seem to update txr->prod without any protection. Seems like you need to take the lock in bnxt_start_xmit_mpc, or get rid of it and assert whatever mutex protects things.

iflibtxrx(9) needs updating for this feature.

Thank you!

Is there another patch which implements the set iotcls? Eg SIOCSIFRSSHASH & SIOCSIFRSSKEY ? How does a per-NIC settable key interact with the global rss hashing config in sys/net/rss_config.c ?

@sumit.saxena_broadcom.com This fixes a bug around ktls handling with mp_ring.
@imp this fixes the bnxt panic you reported to me a few weeks ago

LGTM, but please wait for somebody to review the netlink part, which I did not do.

LGTM, but holding back approval as I don't feel like I own this driver..

I'm confused.. patch looks like its still using taskqueue_drain rather than taskqueue_drain_all

This is really cool. I had never know about fail(9)

Nice! These stats look really interesting.

Also, bnxt_ktls_pre_xmit() and bnxt_ktls_submit_mbuf() both write BDs and advance txr->prod directly, and neither

compares against the consumer.  Is that safe?

Why is the if needed? UINT32_MAX +1 == 0

Can't most these just be implemented in terms of the already existing primitives in sys/bitstring.h? Eg, bnxt_find_first_zero_bit -> bit_ffc bnxt_find_next_zero_bit -> bit_ffc_at bnxt_find_next_bit > bit_ffs_

I think every mpc allocation is leaked on detach. Eg, frees are done from bnxt_queues_free(). iflib runs bnxt_detach(), which sets mpc_info = NULL. And then runs bnxt_queues_free(), which gates freeing on if (!mpc) return;

Does something need to re-apply this after reset?

• In addition to the inline comments, please make the code conform to style(9)
• There do not seem to be licenses on the new files

I spent a while looking at this, and I think maybe we should do a taskqueue_drain_all() here instead. Some drivers (bnxt) use their own tasks registered via iflib_config_task_init that deal with link events and might be subject to the same issue.

In D58266#1336569, @netchild wrote:

This fires too often, maybe some false positives. I will rework.

The whole watchdog thing, like mp_ring, is absurdly over-complicated for no real reason. Just do it the mxge way: if we have pending transmits (eg, producer != consumer) *AND* the nic has not consumed any packets since the last time the watchdog callout ran, then reset. I get dizzy every time i look at the iflib queue-state stuff. Its just extra complexity that adds nothing.

Before and after:

I'd write this as 'mcopydata(m,off, len, mtod(..));

I agree that its unfortunate to burn a flag on this, but in the near term, I think this fixes a real bug and should be committed.

In D57537#1321127, @kp wrote:

In D57537#1319082, @gallatin wrote:

It took me a while, but I think your patch is correct.

@gallatin Do you want to commit this, or should I? (Reid doesn't have a bit, so someone else needs to.)