In D57180#1310528, @pen_lysator.liu.se wrote:

Hmm.. Yeah, I didn't want to change that part of the code. But now when I read the fine print in the man page for strtoul() and strtol() I think we should just get rid of that extra check and just use "*endp != '\0'"...

Hmm.. Shouldn't we use strtoumax() and strtoimax() while we're at it...? Perhaps something like:

intmax_t iv;
uintmax_t uv;
uid_t id;
...
if (*name == -) {
        iv = strtoimax(name,&endp,0);
} else {
        uv = strtoumax(name,&endp,0);
}
if (name == endp || *endp != '\0') {
        errno = EINVAL; /* No or invalid number */
        return (-1);
}
if (*name == '-') {
        id = iv;
        if (int32_t) id != iv) {
               errno = EINVAL; /* Overflow */
               return (-1);
       }
} else {
        id = uv;
        if (id != uv) {
                errno = EINVAL; /* Overflow */
                return (-1);
        }
}

Hmm.. Not sure (or I'm too tired right now) how to write a better and fully portably check for under/overflow in the negative case that works if we get 64 bit uid_t/gid_t. :-)

I'll commit it authored by you, since I don't recall
that you are a committer.

Oh, and I'd leave the comment (or something like it)
in the code, so that it is obvious why the cast'ng is done.

If we cast the ids to (uintmax_t) and then use %ju,
it will still work if uid_t/gid_t ever becomes 64bits.

In D55203#1309629, @kib wrote:

I think it could be set to 'enabled' for machines with DMAP, i.e. amd64/arm64, and might be risc-v, if anybody ever uses nfs server on it. For other arches, the knob almost surely should be kept disabled.
mb_unmapped_to_ext() uses non-privately mapped sfbufs for all extents of mbufs. On DMAP systems, it is free. On other arches, allocating such sfbuf causes global IPI, and the whole chain of sfbufs is freed only after the mbufs are released by the network card. Besides the cost of allocating , this would make sfbufs scarce resource for other consumers and even for nfs server itself. There are around ~1K of sfbufs, and mb_unmapped_to_ext() seems to drop packet if an sfbuf cannot be allocated immediately.

In D55203#1309547, @gallatin wrote:

In D55203#1309439, @rmacklem wrote:

In D55203#1309344, @gallatin wrote:

In D55203#1309194, @glebius wrote:

I don't think this is a right fix. A route lookup now doesn't guarantee the same interface will be used in the future. There is dynamic routing, weighted routing, policy routing, etc etc etc.

There should be some generic gate that would convert mbufs otherwise, we will need to add a code like this every module that generates mbufs. Again, a code that is not correct when routing isn't static.

As Rick said in the description, this is just a hint, and the conversion routine in ip_output() will handle any case where the egress NIC changes. However, just using extpg mbufs all the time seems like a better solution. That's what SW ktls offload does. mb_unmapped_to_ext() is not super expensive, and most high-ish performance NICs are aware of extpgs these days (iflib + mlx5 + cxgbe covers most 10Gb or more NICs in practice).

Kostik preferred not enabling it all the time.
See thtis:
https://lists.freebsd.org/archives/freebsd-net/2026-May/008766.html

The case of concern is some corner case, where
the "always enabled" causes a regression. I asked on freebsd-net@
to try and determine if such a case exists?

Kostik didn't indicate if he thought such a case exists, but was
concerned that the user wouldn't understand why the regression
happened (and the NFS patch includes a sysctl that turns it off).
--> Right now, it is never enabled, so not enabling it does not

 introduce a regression.
Enabling it unconditionally makes a regression more likely
than only enabling it when this hint thinks the NIC can do it
and that hint is incorrect.

I doubt NFS servers have frequent routing changes, but I do not
know that for certain?

You guys can debate it. If there is no consensus, I'll just leave
it disabled as it is now.

I think assuming a performance reduction from mb_unmapped_to_ext() may be a bad assumption. Eg, in my experience from ~2017 or so (when Netflix had enough non-https traffic to matter), using M_EXTPG mbufs for sendfile and doing the conversion at the edge in ip_output() was *faster* than using plain mbufs. I attributed this to avoiding cold-cache pointer chasing in socket buffers. Note that sendfile's use of ext pgs is gated by kern.ipc.mb_use_ext_pgs, which has been 1 since its inception at Netflix. We should probably default it to 1 upstream as well.

In D55203#1309344, @gallatin wrote:

In D55203#1309194, @glebius wrote:

I don't think this is a right fix. A route lookup now doesn't guarantee the same interface will be used in the future. There is dynamic routing, weighted routing, policy routing, etc etc etc.

There should be some generic gate that would convert mbufs otherwise, we will need to add a code like this every module that generates mbufs. Again, a code that is not correct when routing isn't static.

As Rick said in the description, this is just a hint, and the conversion routine in ip_output() will handle any case where the egress NIC changes. However, just using extpg mbufs all the time seems like a better solution. That's what SW ktls offload does. mb_unmapped_to_ext() is not super expensive, and most high-ish performance NICs are aware of extpgs these days (iflib + mlx5 + cxgbe covers most 10Gb or more NICs in practice).

In D55203#1306664, @rmacklem wrote:

In D55203#1306641, @zlei wrote:

In D55203#1305963, @rmacklem wrote:

Could you please take another look at this?
kib@ seems to think this is preferred to just
enabling it for all NICs.

I understand that it is only a "hint", since
routing can change at any time.
However, for an NFS server, routing is
unlikely to change without a NFS server
restart.

Typically a high available NAS server wants link aggregation or ECMP route. The latter may introduce route change if one of the links fails ( planned or un-planned ).

I have a two ports Chelsio T520-CR ethernet card which supports MEXTPG. I'd like to setup an NFS server to test firstly.

Sure. After you have applied this patch, apply the patch here..
https://people.freebsd.org/~rmacklem/new-extpg.patch

I had one person test it who had Mellanox NICs, but no one
who has Chelsio.

Thanks, rick

In D55203#1306641, @zlei wrote:

In D55203#1305963, @rmacklem wrote:

Could you please take another look at this?
kib@ seems to think this is preferred to just
enabling it for all NICs.

I understand that it is only a "hint", since
routing can change at any time.
However, for an NFS server, routing is
unlikely to change without a NFS server
restart.

Typically a high available NAS server wants link aggregation or ECMP route. The latter may introduce route change if one of the links fails ( planned or un-planned ).

I have a two ports Chelsio T520-CR ethernet card which supports MEXTPG. I'd like to setup an NFS server to test firstly.

Could you please take another look at this?
kib@ seems to think this is preferred to just
enabling it for all NICs.

Increase the "off" sanity limit and declare ndm_eocookie
as u_int as suggested by markj@.

Add a sanity limit for "off" to avoid an
overflow when calculating "pos".
This ensures a NULL reply for overly
large offsets.

In D56560#1296776, @bz wrote:

Sorry but I don't know this code specifically and what these macros are doing or not in the end; unless I am going to read it all to understand I cannot help given there is no high level description of what you are trying to (de-)virtualize here.

I had initially asked if it was possible to run NFS exports from within jails. Rick made that happen in 2003(?). And that's kind-of as much as I know. I believe I had not reviewed any of that code back then; you may want to talk to people who did -- I think @markj , @jamie and @mjg did in the end when it came to jails and mounted and some other bits got refined. But that's just from vague memory.

In D56562#1295197, @glebius wrote:

In D56562#1294701, @rmacklem wrote:

For the old way done in FreeBSD-14, the kgss_gssd_handle
would be set NULL when the gssd daemon terminated, at least
when it happened gracefully.

I assume that, for netlink, this never happens?
(ie. Once clnt_nl_create() creates it, it lives forever.)
--> Which means checking for it being non-NULL is a waste

of time?

We have code under #if 0 that implements module unload. It actually should set client to NULL after clnt_destroy(). That's why I left these checks as is, keeping it one change at a time.

However, I agree that we should most likely remove them. As we have rpc_gss module as a proper depend on kggsapi module, and the rpc_gss module always grabs the pointer with kgss_gssd_client() that does CLNT_ACQUIRE()we can actually implement the unload in a manner that through the module lifetime the pointer is never NULL. And module allows unload only when there are no references and no depending modules. But that's definitely a separate patch not to be mixed with this one.

You might want to ask bz@ before getting rid of the
macros, since my recollection was that they were his
idea when I did the code?

I'm fine with getting rid of the macros.

Looks ok to me. If kgss_gssd_handle can never
be NULL, removing the checks for non-NULL
might simplify the code a bit?

This looks fine to me.

Actually that function should go away completely.
It is only used for NFSv4.0, which is now considered
deprecated in favour of NFSv4.1/4.2 and it only
deals with a callback address. Callback addresses
are used for NFSv4.0 only when delegations are
enabled and NFSv4.0 delegations are basically
useless.

Added a sentence w.r.t. when vfs.nfs.enable_uidtostring
needs to be set, as suggested by kib@.

I think I covered kib@'s inline comments.

Looks ok to me. Of course, since we no longer
use Heimdal, I'm not sure it matters?
(Maybe to MFC to FreeBSD-14?)

Thanks for the atomic_load_short explanation.

It all looks good to me, although I won't claim to
understand all the required semantics in
vn_lock_delayed_setsize().

Changed the variable name has_export to only_export,
as suggested by kib@. I also changed the comment
related to this vaiable.

Thanks for doing this. I think it does clarify
the relationship between EXTATTR_NAMESPACE_EMPTY
and EXTATTR_NAMESPACE_EMPTY_STRING for your
other patch.

It looks ok to me, although I do not know
anything about the atf testing part.

I probably should resign, since I don't understand
when/if ACCEPT4_INHERIT might be specified
as an argument for solisten_dequeue()?