This looks correct to me.

In D24552#540209, @hselasky wrote:

Can we use a time-based expiry, like ticks?

In D24408#536859, @rmacklem wrote:

Oh, one other thing.
I can never remember without doing a build, but maybe a
MODULE_DEPEND(xxx, xdr, 1, 1, 1)
needs to be added to all the modules that currently have
MODULE_DEPEND(xxx, krpc, 1, 1, 1)?

In D24408#537610, @freqlabs wrote:

Out of curiosity, how does options XDR (in a kernel config) avoid a name collision with the XDR typedef?

Since separating malloc types goes more and more complicated, follow
Rick's suggestion and abandon that idea. Move MALLOC_DECLARE(M_RPC)
into XDR. Three commits remain:

In D24408#537268, @mmacy wrote:

I don't see anything wrong with this review, but FYI there is now a live upstream https://github.com/openzfs/zfs/commit/9f0a21e6411aa0bac23fba0ddb220342a48c7cc7 and the code in HEAD will some time later this year be replaced with vendor code. Most of the changes to ZFS in HEAD at this point will only live on through MFCs to stable/12 and stable/11.

In D24408#537225, @rmacklem wrote:

It seems that a lot of the complexity of this patch is a result of defining
a new malloc() type.
Is a new type really needed?
I'll leave it up to you, but I'd just stick with M_RPC everywhere and
that would keep the patch a lot simpler.

Address Rick's comment and split into more commits.

In D24408#536854, @rmacklem wrote:

My only concern is that there is a mem_free() call done
somewhere in the rpc code for one of these malloc()s,
but I couldn't find one.
If this happens, it will be easily spotted during testing
when "vmstat -m" generates crap for "rpc" and "xdr".

In D24408#536859, @rmacklem wrote:

Oh, one other thing.
I can never remember without doing a build, but maybe a
MODULE_DEPEND(xxx, xdr, 1, 1, 1)
needs to be added to all the modules that currently have
MODULE_DEPEND(xxx, krpc, 1, 1, 1)?

Sorry, totally forgot about this. Will call them today/tomorrow.

Thanks a lot for the change! I'd really wish that scopes of local variables isn't increased. If it is possible to avoid doing that, I'd really appreciate.

Just a general note, that I really dislike moving of local variables to a greater scope. Yes, I know that a document from the previous century named style(9) suggests to do that. But doing it this way makes code harder to read! Change my mind.

These cards seem to be still produced and sold http://cronyx.ru/hardware/wan.html

In D23788#523247, @neel_neelc.org wrote:

I still get "ngctl: send msg: Invalid argument" errors with the commands in the Test Plan. Am I doing anything wrong?

This can make sense in certain setups. However, since originally node provided writable copies to each of "many" hooks, we can't change that. This can be configured as a node option, if sysadmin is sure that nodes downstream of "many" hooks are fine with read only mbufs.

In D23656#519345, @mjg wrote:

no, keep it grouped with the rest for consistency but #if defined(_KERNEL) around it

Shift down under _KERNEL.

In D23654#519196, @hselasky wrote:

The Mellanox drivers currently manage this internally:
See: https://svnweb.freebsd.org/changeset/base/357294

Usually there are many packets processed per interrupt.

In D23656#519337, @mjg wrote:

looks like this is guarded by _KERNEL in the original header, we should stick to it just in case in the new one

This change looks good to me in general. The only problem I see that Mellanox will now won't have batched epoch. And this was one of the drivers that really benefited from it in our testing of batching epoch at interrupt level.

• Add flag to struct task to mark the task as requiring network epoch.
• Use NET_TASK_INIT() and NET_GROUPTASK_INIT() for drivers that process
• Revert "Temporarily force IFF_NEEDSEPOCH until drivers have been resolved."

Convert pointer in struct task to a flag.

Thanks for the find. I'd rather move the entrance to dyn_tick(). Would use less lines of code and would make it easier to spot it later, when we have callouts supporting epoch.

Since this revision was commandeered, I created a new one: https://reviews.freebsd.org/D23518

Hans, first you start commit war in subversion and then you "commandeer" my revision. We will not make any progress if you "help" like this.

In D23408#513926, @hselasky wrote:

In the kernel there are 403 instances of TASK_INIT(). In this patch I only see NET_TASK_INIT() used 5 times. Does this justify the increased memory usage of "struct task".
grep -r TASK_INIT\( sys/ | grep -v GTASK | wc -l

403