Re: ALTQ

[Jeffrey Hsu <hsu@xxxxxxxxxxx>]

Joerg Sonnenberger wrote:
> On Thu, Feb 10, 2005 at 10:18:45AM -0800, Matthew Dillon wrote:
>
> >    It looks ok except for the various IFQ_*() macros.  Passing a variable
> >    name as an argument to a macro (e.g. err) which then assigns the 
> >    variable creates a lot of confusion.  The NFSM code did this (albeit 
> >    much more aggregiously) and it created a real mess.
>
>
> It still makes the variable used for error handling explicit.
> The macros are compatible with KAME, I'd prefer to keep the current form.
> They are used in two cases which a normal network driver should not trigger.
> re(4) is a bit special because it tries to test the card first.

I wouldn't worry about vendor compatability.  We can also try to get the
changes back to KAME or we could import the KAME code on a vendor branch
and deal with vendor changes using cvs.

> >    Could you make those IFQ_*() macros real procedures instead of macros?
> >    Or at least real inline procedures.
>
>
> Real functions is not an option, because it would enforce the overhead
> for all interfaces, independent of ALTQ being available or not. Inline
> functions would work, but still need a macro since IFQ_ENQUEUE's pattr
> attribute is not available in non-ALTQ code. I'm not sure if that makes
> the code really any easier to read.

If you're worried about compile-time efficiency, I would use the following,
which uses the compiler to optimize out the non-ALTQ code, while allowing
full compile-checking even when ALTQ is not defined.  It consolidates the
ALTQ and non-ALTQ cases into 1 inline version.

From

#ifdef ALTQ

#define IFQ_ENQUEUE(ifq, m, pattr, err) do {                            \
       if (ALTQ_IS_ENABLED((ifq)))                                     \
               ALTQ_ENQUEUE((ifq), (m), (pattr), (err));               \
       else {                                                          \
               if (IF_QFULL((ifq))) {                                  \
                       m_freem((m));                                   \
                       (err) = ENOBUFS;                                \
               } else {                                                \
                       IF_ENQUEUE((ifq), (m));                         \
                       (err) = 0;                                      \
               }                                                       \
       }                                                               \
       if ((err))                                                      \
               (ifq)->ifq_drops++;                                     \
} while (0)

#else

#define IFQ_ENQUEUE(ifq, m, pattr, err) do {                            \
       if (IF_QFULL(ifq)) {                                            \
               m_freem(m);                                             \
               (err) = ENOBUFS;                                        \
       } else {                                                        \
               IF_ENQUEUE(ifq, m);                                     \
               (err) = 0;                                              \
       }                                                               \
       if (err)                                                        \
               (ifq)->ifq_drops++;                                     \
} while (0)

#endif

#define IFQ_HANDOFF(ifp, m, pattr, err) do {                            \
       int s;                                                          \
                                                                       \
       s = splimp();                                                   \
       IFQ_ENQUEUE(&(ifp)->if_snd, m, pattr, err);                     \
       if ((err) == 0) {                                               \
               (ifp)->if_obytes += (m)->m_pkthdr.len;                  \
               if ((m)->m_flags & M_MCAST)                             \
                       (ifp)->if_omcasts++;                            \
               if (((ifp)->if_flags & IFF_OACTIVE) == 0)               \
                       (*(ifp)->if_start)(ifp);                        \
       }                                                               \
       splx(s);                                                        \
} while (0)

to

#ifndef ALTQ
#define ALTQF_ENABLED 0
#endif

static int __inline
ifq_enqueue(struct ifqueue *ifq, struct mbuf *m, struct altq_pktattr *pattr)   
{
       int error;

       if (ifq->altq_flags & ALTQF_ENABLED) {                 
               error = (*ifq->altq_enqueue)(ifq, m, pattr);
       } else {                        /* non-altq case */
               if (if_qfull(ifq)) {             
                       m_freem(m);
                       error = ENOBUFS;
               } else {       
                       if_enqueue(ifq, m);
                       error = 0;
               }
       }
       if (error)                                                        
               ifq->ifq_drops++;                                      
       return error;
} 

static int __inline
ifq_handoff(struct ifnet *ifp, struct mbuf *m, struct altq_pktattr *packetattr)
{
       int error, s;

       s = splimp();
       error = ifq_enqueue(&ifp->if_snd, m, packetattr);
       if (error == 0) {
               ifp->if_obytes += m->m_pkthdr.len;
               if (m->m_flags & M_MCAST)
                       ifp->if_omcasts++;
               if (!(ifp->if_flags & IFF_OACTIVE))
                       (*ifp->if_start)(ifp);
       }
       return error;
}