dpaa2: Clean up channels in separate tasks

[dsl]

Summary

Each channel gets its own DMA resources, cleanup and "bufferpool"
tasks, and a separate cleanup taskqueue to isolate channels operation
as much as possible to avoid various kernel panics under heavy network
load.

As a side-effect of this work, dpaa2_buf structure is simplified and
all of the functions to re-seed those buffers are gathered now in
dpaa2_buf.h and .c files; functions to work with channels are
extracted into dpaa2_channel.h and .c files as well.

Test Plan

https://github.com/mcusim/freebsd-src/issues/19
https://github.com/mcusim/freebsd-src/issues/20

[bz]

In a lot of ways I like breaking the code out.
The amount of changes (including "whitespace noise") makes it extremely hard/time consuming to review.

In theory this should be broken up into multiple commits: first move the code as is into extra files (no functional changes), then reduce code duplication etc, then rename variables etc without other functional changes, ... a dedicated cleanup change, ... I know this is hard and a lot of extra work. Please try to get to that in the future. It helps a lot with review, keeping functional changes smaller, also when going back in history and looking at a change again or when bisecting for other reasons.

For this one I'd take it as a "vendor" update in one big go still. Would be good to rephrase and wrap the proposed commit message a bit. I'd probably have the kernel panics and item 3 together and the factoring out into different files as a "side-effect" of restructuring code for 3. and maintenance? ( I am guessing a bit here, you know better ).

sys/conf/files.arm64

219

I assume the module Makefile also needs to learn about the new files?

sys/dev/dpaa2/dpaa2_channel.h

71

I cannot make my head up if I like this kind of "debugging" the assertion or not.
It feels wrong to me.

sys/dev/dpaa2/dpaa2_ni.c

2507

Loosing a packet is undesirable but it's a network. If resources don't allow you, don't try to be smart. Simply drop it. If it was important it'll be resend. Otherwise apply any UDP joke ;-) I'd remove the panic in the INVARIANTS case too.

2861

And how do you currently prevent that?

3075

I would print at least 'rc' in that case so you know which error you were not expecting but got back.

sys/dev/dpaa2/dpaa2_rc.c

45

Seems unrelated cleanup..

sys/dev/dpaa2/dpaa2_swp.h

323

Leaves you with an empty union {}; probably put that inside the notyet as well to avoid compiler noise.

sys/dev/dpaa2/dpaa2_types.h

71

This entire interface smells like it wants to be sys/refcnt.h or sys/counter.h (not sure if the xchg case is just an optimized SUB/ADD in a loop case)?

[dsl]

In theory this should be broken up into multiple commits: first move the code as is into extra files (no functional changes), then reduce code duplication etc, then rename variables etc without other functional changes, ... a dedicated cleanup change, ... I know this is hard and a lot of extra work. Please try to get to that in the future. It helps a lot with review, keeping functional changes smaller, also when going back in history and looking at a change again or when bisecting for other reasons.

I'd keep this review as a parent for those smaller changes if you want. Sounds like a good exercise for me and a chance for you to better review changes taking upcoming 14.0 into account.

[dsl]

sys/dev/dpaa2/dpaa2_channel.h

71

I've commented it below.

sys/dev/dpaa2/dpaa2_ni.c

2861

I've found this note and decided to give channels not only their cleanup tasks, but single threaded taskqueues as well:

	ch->cleanup_tq = taskqueue_create("dpaa2_ch cleanup", M_WAITOK,
	    taskqueue_thread_enqueue, &ch->cleanup_tq);
	taskqueue_start_threads_cpuset(&ch->cleanup_tq, 1, PI_NET,
	    &iosc->cpu_mask, "dpaa2_ch%d cleanup", ch->id);

My idea to panic as soon as the storage mutex isn't available right now would indicate an error only if the taskqueue behavior not to enqueue already running task on the same thread will somehow be broken in future, but DPAA2 bits would be among other broken pieces, I guess. I'll remove this "indication".

[dsl]

sys/dev/dpaa2/dpaa2_ni.c

2507

OK, sounds reasonable 😃

[dsl]

sys/dev/dpaa2/dpaa2_types.h

71

from sys/arm/include/atomic.h:

static __inline uint32_t
atomic_swap_32(volatile uint32_t *p, uint32_t v)
{
	uint32_t ret, exflag;

	__asm __volatile(
	    "1: ldrex	%[ret], [%[ptr]]		\n"
	    "   strex	%[exf], %[val], [%[ptr]]	\n"
	    "   teq	%[exf], #0			\n"
	    "   it	ne				\n"
	    "   bne	1b				\n"
	    : [ret] "=&r"  (ret),
	      [exf] "=&r" (exflag)
	    : [val] "r"  (v),
	      [ptr] "r"  (p)
	    : "cc", "memory");
	return (ret);
}

refcount(9) looks suitable. Do you mind if I'll switch to it in a separate review?

[dsl]

Commit message's updated.

[bz]

I'll try to have this checked to some degree at least by tomorrow evening.

sys/dev/dpaa2/dpaa2_types.h

71

I was inquiring about the implementation of swap, I was more enquiring about what your code does with ith.
Yes, changing separate sounds fine.

sys/modules/dpaa2/Makefile

16 ↗

(On Diff #125931)

Thanks!

23 ↗

(On Diff #125931)

The changes below seem whitespace noise. I am fine in this larger change with it but in general "cleanup" stays separate. Commits are cheap.

[dsl]

sys/modules/dpaa2/Makefile

23 ↗

(On Diff #125931)

OK, understood.

[dsl]

Commit message has been updated and $FreeBSD$ removed from new files only.

[bz]

Your rebase wasn't clean: https://reviews.freebsd.org/D41296?vs=125931&id=126155#toc

[dsl]

@bz I'm not exactly sure why phabricator shows diffs this way, but I don't see those lines changed between main and dpaa2 branches: https://github.com/freebsd/freebsd-src/compare/main...mcusim:freebsd-src:dpaa2

Aren't those changes visible because it wasn't only me who modified those files, but Warner's script which removes $FreeBSD$ as well? And they are here because phabricator shows diffs between the previous revision of mine and the current one after rebase, i.e. I've re-based dpaa2 branch correctly.

[bz]

In D41296#945547, @dsl wrote:

@bz I'm not exactly sure why phabricator shows diffs this way, but I don't see those lines changed between main and dpaa2 branches: https://github.com/freebsd/freebsd-src/compare/main...mcusim:freebsd-src:dpaa2

Aren't those changes visible because it wasn't only me who modified those files, but Warner's script which removes $FreeBSD$ as well? And they are here because phabricator shows diffs between the previous revision of mine and the current one after rebase, i.e. I've re-based dpaa2 branch correctly.

The main branch where you run arc is not at the same hash as the rebase of your branch. This is local and has nothing to do with github. Assume you'd want to push and you need to update your local branch to the latest main. So
git checkout main && git fetch freebsd && git merge --ff-only freebsd/main
git checkout D41296 && git rebase -i main && git diff main && arc diff --update D41296

The name of the commit branch is assumed; but that should show a clean diff which could be pushed.

[dsl]

Re-based on the latest main.