Why are these lines changed/re-arranged.. is there some reason for it?

Your comment above
"SMR section makes it safe to access a pcb after it went through uma_zfree(). We always want a particular pcb locked, so after successful lookup and lock we just check INP_FREED."

confuses me here. The comment implies that we need to check INP_FREED, but yet here you remove that. Is the comment wrong or misleading? Did you mean we no-longer check INP_FREED maybe??

These changes here appear to be un-related to the work described.

Conceptually the INP_WLOCK() should be done before line 619. I know its not
"lookup-able" at this point but I think for consistency you should not be writing
anything on the inp here without holding the lock.

Why are we removing the spare pointer?

This comment seems to be incomplete like you stopped in the middle of a sentence was more intended?

The order in the table determines the lock order expected by witness.

Looks like there's a missing newline here.

refcount_acquire() returns the old value, so you could use that to write a non-racy assertion:

u_int old __diagused;

old = refcount_acquire(&inp->inp_refcount);
KASSERT(old > 0, ...);

No need for this assertion, refcount_release() will panic in this case.

Consider moving the assertions into a subroutine so they aren't duplicated.

Is this unfinished?

proc.h sorts before rwlock.h

same for the others above.

Could be just return (inp->inp_lport == nport);.

The locks with not-so-informative names "udp" and "tcp" correspond to the big struct pcbinfo locks. The locking order has changed. Now we no longer use pcbinfo lock to traverse the lists, however when we free or alloc a pcb, we would grab info lock holding a pcb lock.

P.S. I think these WITNESS hint blocks can be removed as a whole. They are needed only for complicated cases, and this case is no longer a complicated one. WITNESS will learn the right order naturally.
P.P.S. In the current patch there are couple places where we use the old order (inp_apply_all, in_pcb_notifyall), this will be addressed in next update of the review.

The comment belongs to in_pcb.c only. When we came to a pcb via a CK list pointer, then we must check this flag, and this is what inp_smr_lock() or inp_list_next() do.

Nobody outside in_pcb.c shall come to a pcb via CK list pointers.

Yep, and they already are committed to FreeBSD/main.

When I rebase the patch, they will disappear. But I'm not rebasing above our Netflix merge point, cause that would break my merging of this branch to a Netflix branch. So they will disappear next week.

That's a good conceptual question! It should be either locked right after allocation and even before bzero, cause bzero already writes to the protected fields OR it should be locked right before it could be looked up. I'll leave that open for now and see what others think.

P.S. I will also ask Drew, what if such change can be not only stylistic but affect performance?

Will do. Thanks!

The HPTS-related ones will go away later. So only two MPASS will remain. I'll leave as is for now, if you don't mind.

Will fix, thanks!

The spare pointers are a remnant of times when inpcb was exported to userland. I'll put it back, to separate structure cleanup from the main commit.

Will fix!

Missing newline. To be fixed in next update.

inpcbzone_name can be const, BTW.

These backslashes aren't needed.

Maybe call this advance or next_locked instead of next.

There is no guarantee that next is a valid inp at this point. While outside of the SMR section, next could have been freed and reallocated, so it may belong to a different hash bucket, or it may have been moved to the end of the global list (so some elements can be skipped), or the backing slab could have been reallocated for some other purpose (inps are not type-stable), so next is a pointer to random memory.

More than that can be improved here. I got a patch in queue that would static-ize most of struct inpcbinfo initialization. Don't want to pile it into this review.

That's a very good catch. Thanks! I will look deeper into here.

This line is erroneous. Should have gone with the last update. It will be deleted in next update.