Details

Reviewers

bz
andrew

Commits

rG58983e4b0253: dpaa2: Clean up channels in separate tasks

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dsl created this revision.Aug 3 2023, 1:54 PM

Herald added a reviewer: andrew. · View Herald TranscriptAug 3 2023, 1:54 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added subscribers: emaste, imp. · View Herald Transcript

dsl requested review of this revision.Aug 3 2023, 1:54 PM

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
72	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
2505	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.
2853	And how do you currently prevent that?
3056	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 ↗	(On Diff #125499)	Seems unrelated cleanup..
sys/dev/dpaa2/dpaa2_swp.h
324	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)?

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 added inline comments.Aug 7 2023, 10:15 AM

sys/dev/dpaa2/dpaa2_channel.h
72	I've commented it below.
sys/dev/dpaa2/dpaa2_ni.c
2853	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 updated this revision to Diff 125804.Aug 10 2023, 9:21 AM

dsl marked 3 inline comments as done.

dsl added inline comments.

sys/dev/dpaa2/dpaa2_ni.c
2505	OK, sounds reasonable 😃

dsl marked an inline comment as done and 2 inline comments as not done.Aug 10 2023, 9:25 AM

dsl added inline comments.Aug 10 2023, 10:20 AM

sys/dev/dpaa2/dpaa2_types.h

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 updated this revision to Diff 125931.Aug 12 2023, 10:07 AM

dsl edited the summary of this revision. (Show Details)

Commit message's updated.

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	Thanks!
23	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 added inline comments.Aug 16 2023, 9:27 AM

sys/modules/dpaa2/Makefile
23	OK, understood.

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

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

This revision now requires changes to proceed.Aug 17 2023, 9:37 PM

@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.

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.

Re-based on the latest main.

bz accepted this revision.Aug 19 2023, 3:36 AM

This revision is now accepted and ready to land.Aug 19 2023, 3:36 AM

Closed by commit rG58983e4b0253: dpaa2: Clean up channels in separate tasks (authored by dsl). · Explain WhyAug 20 2023, 8:18 AM

This revision was automatically updated to reflect the committed changes.

dsl added a commit: rG58983e4b0253: dpaa2: Clean up channels in separate tasks.

dpaa2: Clean up channels in separate tasks
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 126204

sys/conf/files.arm64

sys/dev/dpaa2/dpaa2_buf.h

sys/dev/dpaa2/dpaa2_buf.c

sys/dev/dpaa2/dpaa2_channel.h

sys/dev/dpaa2/dpaa2_channel.c

sys/dev/dpaa2/dpaa2_io.h

sys/dev/dpaa2/dpaa2_io.c

sys/dev/dpaa2/dpaa2_mc.c

sys/dev/dpaa2/dpaa2_mcp.h

sys/dev/dpaa2/dpaa2_mcp.c

sys/dev/dpaa2/dpaa2_ni.h

sys/dev/dpaa2/dpaa2_ni.c

sys/dev/dpaa2/dpaa2_swp.h

sys/dev/dpaa2/dpaa2_swp.c

sys/dev/dpaa2/dpaa2_types.h

sys/dev/dpaa2/dpaa2_types.c

sys/modules/dpaa2/Makefile

dpaa2: Clean up channels in separate tasksClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 126204

sys/conf/files.arm64

sys/dev/dpaa2/dpaa2_buf.h

sys/dev/dpaa2/dpaa2_buf.c

sys/dev/dpaa2/dpaa2_channel.h

sys/dev/dpaa2/dpaa2_channel.c

sys/dev/dpaa2/dpaa2_io.h

sys/dev/dpaa2/dpaa2_io.c

sys/dev/dpaa2/dpaa2_mc.c

sys/dev/dpaa2/dpaa2_mcp.h

sys/dev/dpaa2/dpaa2_mcp.c

sys/dev/dpaa2/dpaa2_ni.h

sys/dev/dpaa2/dpaa2_ni.c

sys/dev/dpaa2/dpaa2_swp.h

sys/dev/dpaa2/dpaa2_swp.c

sys/dev/dpaa2/dpaa2_types.h

sys/dev/dpaa2/dpaa2_types.c

sys/modules/dpaa2/Makefile

dpaa2: Clean up channels in separate tasks
ClosedPublic
Actions

Revision Contents
Changeset List