On Mar 27,  1:28am, manu@netbsd.org (Emmanuel Dreyfus) wrote:
-- Subject: Re: fixing send(2) semantics (kern/29750)

| Christos Zoulas <christos@zoulas.com> wrote:
| 
| > 1. Don't call the variable sleep. It is not a good idea to name variables
| >    after commonly used functions. And you get shadow warnings too.
| > 2. Probably it is time to IFQ_ENQUEUE -> ifq_enqueue()... a function.
| > 3. If non-blocking I/O is set then you should probably return EWOULDBLOCK
| >    or EAGAIN. How do the upper layers handle this (i.e. does the operation
| >    get really retried or the packet is dropped and is handled by the prorocl
| >    layer?)? Does using select work to wakeup when space is available again?
| 
| That's the whole point: if non blocking I/O is set, SUS and our man
| pages say that we must block instead of failling.

The semantics of non-blocking I/O are the same across devices. If non
blocking I/O is set, and there is no room, then the application should
return EAGAIN instead of sleeping. When that happens the application
can use select or poll to sleep until there is more room. Is that the
case here? I don't think so.

| > 4. There is a typo in "should"
| > 5. if_ppp.c if_strip.c if_sl.c if_sppsubr.c open-code the ALTQ code. I don't
| >    think that this is needed anymore.
| 
| What do you mean?

        if ((m0->m_flags & M_HIGHPRI)
#ifdef ALTQ
            && ALTQ_IS_ENABLED(&sc->sc_if.if_snd) == 0
#endif
            ) {
            ifq = &sc->sc_fastq;
            if (IF_QFULL(ifq) && dst->sa_family != AF_UNSPEC) {  
                IF_DROP(ifq);
                splx(s);
                error = ENOBUFS;
                goto bad;
            } else {
                IF_ENQUEUE(ifq, m0);
                error = 0;
            }
        } else
            IFQ_ENQUEUE(&sc->sc_if.if_snd, m0, &pktattr, error);

| 
| > 6. As far as passing in the spl level, I think that the whole thing should
| >    be done in one function:
| > 
| >       mflags = m->m_flags;
| >       len = m->m_pkthdr.len;
| >       s = splnet();
| >       /*
| >        * Queue message on interface, and start output if interface
| >        * not yet active.
| >        */
| >       IFQ_ENQUEUE(&ifp->if_snd, m, &pktattr, error);
| >       if (error) {
| >               /* mbuf is already freed */
| >               splx(s);
| >               return (error);
| >       }
| >       ifp->if_obytes += len;
| >       if (mflags & M_MCAST)
| >               ifp->if_omcasts++;
| >       if ((ifp->if_flags & IFF_OACTIVE) == 0)
| >               (*ifp->if_start)(ifp);
| >       splx(s);
| > 
| >     Since all the non-tunneling interfaces do this, or a buggy version
| >     of this. Then you can have a different version for the tunnelling
| >     interfaces (bridge, gif, etc.). This will cleanup the code significantly.
| 
| Right. I call it ifq_enqueue, I expand IFQ_ENQUEUE in it, and I remove
| IFQ_ENQUEUE definition?

Right, but you need to take care of the bridge, gif etc. which use IFQ_ENQUEUE
slightly differently. That is why you probably need two versions of the code.

| >     Also the ieee1394 appears to freeing the mbuf on failure, or am I
| >     mis-reading this?
| 
| Maybe a bug, who did the firewire support?

Dunno. I would leave it alone for now. Anyway we are going to probably
replace it soon.

christos