Though, part of the problem with this patch is that I'm not entirely sure with what I want the parameters of ift_txq_select to be.

Right now it takes the mbuf pointer which is enough for what we want to do with it in an upcoming ice(4) patch, but in the future I'll need to use DSCP/IP ToS information in that function as well. I think I should be able to put that information inside the mbuf along with something like an M_DSCP flag. Though, one version I had before also had the if_pkt_info_t struct as well as having the headers (partially) parsed before this queue selection function was called in order to avoid having to modify mbufs like that.

In D31485#754650, @erj wrote:

Though, part of the problem with this patch is that I'm not entirely sure with what I want the parameters of ift_txq_select to be.

Right now it takes the mbuf pointer which is enough for what we want to do with it in an upcoming ice(4) patch, but in the future I'll need to use DSCP/IP ToS information in that function as well. I think I should be able to put that information inside the mbuf along with something like an M_DSCP flag. Though, one version I had before also had the if_pkt_info_t struct as well as having the headers (partially) parsed before this queue selection function was called in order to avoid having to modify mbufs like that.

Either seems reasonable to me. I would bias toward including the if_pkt_info since it has fields for offload handling but I don't know much about other expected use cases, maybe @jhb has some thoughts from crypto and tcp offloads.

In D31485#756141, @kbowling wrote:

Either seems reasonable to me. I would bias toward including the if_pkt_info since it has fields for offload handling but I don't know much about other expected use cases, maybe @jhb has some thoughts from crypto and tcp offloads.

Yeah, I think his input would be good. Maybe it's worth discussing what I want to do in the future here, too.

I looked through more of the kernel to try to find a good approach, but they all seem like they could be problematic, performance-wise:

• I thought maybe storing the DSCP value in the mbuf would work, but the first problem is that we seem to be out of M_* flags; there are no spare flags left, and all of the remaining M_PROTO flags are used up by ieee80211 it seems. It looks like some of the code that stores VLAN PCP information uses mbuf tags, so that could be an option, but I'd have to look at every place in the code that sets the IP ToS/DSCP information and wrap that with a function that creates an mbuf tag (and similarly to the vlan_mtag_pcp stuff here, probably enable/disable that with a sysctl). Though, my gut feeling is that this seems like it could have a large negative impact on performance.
• Instead, dealing with extracting the DSCP value from the packet inside iflib or the driver would be less invasive, but like I said in my previous comment, the thread that initially queues the packet onto a per-queue thread would have to do more work to parse headers to get that value. Though, that may be partially mitigated by the fact that we only need to peer into the outermost L3 header, which may not be too bad and might not require an m_pullup() in most cases. But that m_pullup cruft still needs to be called in there to make sure we can read at least the minimal IPv4/IPv6 header.
  • We could maybe avoid having to have the if_pkt_info_t in the function parameter list by allocating an mbuf tag or using the M_PROTO flags and empty PH_loc space to indicate a valid DSCP value exists. But, there might be more information in there that we could use in the future that might not fit into an mbuf. e.g. maybe something ADQ-like where we check flows and map the flows to one of the 16k Tx queues in ice(4) hardware in the future. I wanted to avoid adding a new parameter because we're already releasing an ice(4) driver that expects the function signature used in this patch, and it wouldn't be feasible to change it. But if we absolutely have to, then it's better to do it early I suppose.

And I think both of these options have performance impacts, but I'm not sure how to quantify them properly. For now, I'm working on patches for both approaches, and I already have a draft of the latter.

Everyone here (at work) is mostly going to be on vacation for the next couple weeks, so we probably won't do much with this patch until people (and me) come back.

We're back, and I think I want to get this committed soon because we're about ready to start adding reviews for the ice(4) update that uses this, as well as upstreaming the FreeBSD RDMA driver. Maybe I'll have to worry about the DSCP changes after this patch.

If no one has any objections, then I'll update this patch with a new FreeBSD version number, upload that here, and commit it. Then, I can submit a follow-on ice(4) update that uses it.