Convert nfs to use the IfAPI

[jhibbits]

Summary

Use the new IfAPI interface and address iterators so the nfs driver
doesn't need direct access to the interface structures.

[jhibbits]

Enter the net epoch instead of using the ifnet lock.

[melifaro]

sys/nfs/bootp_subr.c

1541

Isn't it just if_getifaddr(ifp) ?

sys/nfs/nfs_diskless.c

157

Nit: maybe a bit more descriptive name? It's a bit hard (at least for me) to understand what cbs is.
Also: do we need to propagate struct nfs_setup_ifa_s here instead of just ourdl?

238

Nit: maybe use C11 initializer here?

[jhibbits]

sys/nfs/bootp_subr.c

1541

I'm not sure. It's looking specifically for an IFT_ETHER type, would the if_addr always be an IFT_ETHER?

sys/nfs/nfs_diskless.c

157

We need to pass down 'ourdl', and pass back up 'ifp'. The API doesn't provide 'in' vs 'out'.

157

It would be nice to have lambdas or blocks in the kernel, since most of these callback loops end up being rather trivial.

[melifaro]

sys/nfs/bootp_subr.c

1541

if_attach() creates a new sdl, which is then assigned to if_addr. The data inside this sdl is derived from the if_type at the moment of attach (e.g. for everrything calling ether_ifattach() it will be IFT_ETHER).
There is also if_vlan which manually re-sets if_type and sdl->sdl_type if ITF_L2VLAN, but that doesn't change the invariant (sdl type == ifp->if_type).

Additionally, I'm not sure if having AF_LINK address in the chain of addresses is the right thing to have.
The only purpose seem to provide the convenient interface to getifaddrs(3), but I see quite a bit of complexity added to other places in the kernel (basically, each address iterator has != AF_LINK case).

sys/nfs/nfs_diskless.c

157

Sorry for not being clear - I meant the following:

static u_int
nfs_setup_diskless_ifa_cb(void *arg, struct sockaddr_dl *sdl, u_int count)
{
	struct sockaddr_dl *outdl = arg;
...

count = if_foreach_lladdr(ifp, nfs_setup_diskless_ifa_cb, cbs->ourdl);

157

I'd love to have that. Not with C, unfortunately.
I toyed with the idea of providing iterator-based KPI instead of callbacks for routing.
I didn't come with anything I'm comfortable with, mostly due to the complexity of restoring radix iteration state. However, it may be possible to have something workable for ifp/ifa.

[rmacklem]

I'm afraid I don't understand the diskless stuff related
to network configuration. I don't even have a diskless
setup to test with.

As such, I can't really be of any help.

Good luck with it, rick

[melifaro]

sys/nfs/nfs_diskless.c

157

Looked into the iterator part once again.
Wild idea - something like that can work for ifp/ifaddrs.

Technically it can be simpler (just using if_next() with the iterator struct zeroed initially), but I see two benefits in this approach:

1. it is not too-hard tied to the current implementation, allowing to switch implementation transparently
2. KPI approach (explicit iterator creation/deletion) will be consitent with rtable iterators I'm going to introduce.

What do you think?

Public KPI:

struct if_iter {
  void *_ptr[4];
}
void if_get_iter(struct if_iter *iter);
struct ifnet *if_next(struct if_iter *iter);
void if_free_iter(struct if_iter *iter);

Usage:

struct if_iter iter;
if_get_iter(&iter);
while ((struct ifnet *ifp = if_next(&iter)) != NULL) {
 ...
}
if_free_iter(&iter);

Implementation:

struct if_iter_private {
  struct ifnet *cur_ifp;
  struct ifnet *next_ifp; 
};
Static_assert(sizeof(struct if_iter_private) < sizeof(struct if_iter));

void
if_get_iter(struct if_iter *_iter)
{
  NET_EPOCH_ASSERT();
  struct if_iter_private *iter = _iter;

  bzero(iter, sizeof(*iter));
  iter->cur_ifp = CK_STAILQ_FIRST(&V_ifnet, if_link);
  if (iter->cur_ifp != NULL)
   iter->next_ifp = CK_STAILQ_NEXT(iter->cur_ifp, if_link);
}

struct ifnet *
if_next(struct if_iter *_iter)
{
  struct if_iter_private *iter = _iter;
  struct ifnet *ifp = iter->cur_ifp;
  iter->cur_ifp = iter->next_ifp;
  if (iter->cur_ifp != NULL)
    iter->next_ifp = CK_STAILQ_NEXT(iter->cur_ifp, if_link);

  return (ifp);
}

void
if_free_iter(struct if_iter *iter __unused)
{
}

[jhibbits]

@melifaro I like that approach overall, but I think it's nicer to keep it all local on the stack using an opaque context struct, then a user can do something like:

struct if_iter_t i;
for (if_t ifp = if_iter_start(&i); ifp != NULL; ifp = if_next(i)) {
  do_something(ifp);
}

This removes the if_free_iter() KPI you propose, until such a time comes that the iterator context blows out some set size (4 pointers as you had created).

[jhibbits]

Update.

[jhibbits]

Ping anyone?