In D39805#906206, @markj wrote:

Would anyone be willing to test this on an AMD system?

up

In D39369#896103, @zlei wrote:

Generally looks good to me.

The patch slightly increases RX PPS: ~ 100 Kpps.

From to ?

@kp , just out of curiosity. How many packets per second did you get in your test after this patch?

@vmaffione ?

Thanks. I will submit this patch in a separate review where we can discuss the details.

My main objection is that it is bad to extend 'struct protosw' for only one consumer ( ng_kocket(4) ).

In D36451#828133, @mjg wrote:

can i get some feedback here? the netgraph panic is partially blocking other work

@rew, do you have any plans to continue this work? If not, I'd like to take over.

Looks good to me.

I'm concerned about two things:

I have this problem in STABLE-13 with disk image located on NFS share.

@vmaffione , do you have any objections?

In D34260#775362, @markj wrote:

BTW, mdstart_vnode() does not set bio_resid after handling a BIO_FLUSH. I suspect it does not matter in practice, since by convention BIO_FLUSH commands have bio_length == 0.

In D34185#773670, @vmaffione wrote:

Question: there are three more locations in that source file where NG_SEND_DATA_ONLY() is called and it is not apparent that we are inside NET_EPOCH.
Should we enter NET_EPOCH also there, or we are already in?

In D26268#756453, @markj wrote:

Should this be committed?

I don't understand what the alignment does in this case.
Does it fix a bug or is it an optimization?
What part of the code requires this alignment, is it the TCP/IP stack?
Because not only ng_eiface(4) can inject Ethernet packets into the network stack.

The polynomial 0x8005 is CRC-16-IBM or CRC-16-ANSI.

This is done in FreeBSD 13.

I am also against the use of generic messages.

I looked at other nodes that implement the ng_newhook_t method. They already use M_NOWAIT, so I think this patch is good.

The load simply allows this bug to manifest itself. Under load, the node's queue already contains the item's, so if a message item is sent at that moment (ngtstl shutdown), it will also go to the queue and will be processed by ngthread(). Without load, the queue is mostly empty, the message item will be delivered/processed by the thread of the calling process directly: ngctl -> syscall (sandto(2)) -> ngc_send (ng_socket(4)) -> ng_eiface(4) without entering to the EPOCH section. Therefore, the bug does not manifest itself.

JFYI, the same situation. If you call "ngctl shutdown ngeth0:" under load:

I think the problem is somewhat broader. Any message item can be added to the node's queue under load or if the queue is blocked. This message item will be processed by ngthread() in the EPOCH section. There are many places in the code of the nodes that perform actions prohibited in the EPOCH section.

Overall, this patch looks good to me.

In D30638#688320, @np wrote:

I know it's not directly related but the 'if' here is not needed. If you're tightening the len checks in the function then maybe clean this too?

out:

if (m != NULL)
        m_freem(m);

• Move ethernet packet length check to vxlan_input().

I prefer having sound loaded as a module in fact, but until we have module autoloading by PCI ID this should be available in GENERIC.

The capsicum support should look something like this:

Have you tested this code with CAPSICUM enabled?

As I mentioned previously, the other thing to consider when using kvmclock in the current patch is the need for a system call for clock_gettime(), gettimeofday(), etc.

@me_freebsd_mathieu.digital, what a CPU model was used for testing?

I have no objections.

I'm not an English expert, so you may be right.
Technically, I have no questions, I think this patch can be committed.

Did you mean D28559?

I have no competence in this area. Maybe you wanted to add @fsu to the reviewers? He was working on improving ext2/3/4 support.

Lutz, do you have any plans for the upcoming changes?
I also thought about getting rid of ng_bridge from NG_NODE_FORCE_WRITER. Since rcv_data is always called in the NET_EPOCH context, I think we can do it like @kp did it for if_bridge(4) (see D24250).

I don't really like how ng_etner(4) is implemented as a whole. But I think this patch is correct and should be committed.

@grehan do you have any objections?