On Mon, Jan 23, 2017 at 10:10:23AM +1000, Martin Pieuchot wrote:
> On 22/01/17(Sun) 20:38, David Gwynne wrote:
> > ifqueues count packets, therefore if_enqueue doesnt have to.
> >
> > this adds reading of the ifq counters into the if_data values so
> > userland still looks right.
> >
> > this is another step toward multiple tx queues.
>
> I like it, some comments below.
>
>
> > @@ -1799,10 +1790,13 @@ ifioctl(struct socket *so, u_long cmd, c
> > ifr->ifr_hardmtu = ifp->if_hardmtu;
> > break;
> >
> > - case SIOCGIFDATA:
> > - error = copyout((caddr_t)&ifp->if_data, ifr->ifr_data,
> > + case SIOCGIFDATA: {
> > + struct if_data data;
>
> I'd use ifdata that would avoid shadowing the 'data' pointer.
ok.
> > + error = copyout((caddr_t)&ifp->if_data, &data,
> > sizeof(ifp->if_data));
>
> Shouldn't this be:
>
> copyout(&data, ifr->ifr_data, sizeof(data))
yes.
> > +void
> > +if_data(struct if_data *data, struct ifnet *ifp)
>
> Could you pass ``ifp'' as first argument to keep coherency with the rest
> of if.c?
i can. i thought of it as a memcpy like function, so dst src.
fixed.
> > + /* it's ugly reaching into an ifq like this */
>
> What's ugly? I'm not sure this comment help.
if.c doesnt know much about ifq internals, except for this bit.
Index: if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.475
diff -u -p -r1.475 if.c
--- if.c 22 Jan 2017 10:17:39 -0000 1.475
+++ if.c 23 Jan 2017 00:32:53 -0000
@@ -583,8 +583,7 @@ if_start_locked(struct ifnet *ifp)
int
if_enqueue(struct ifnet *ifp, struct mbuf *m)
{
- int length, error = 0;
- unsigned short mflags;
+ int error = 0;
#if NBRIDGE > 0
if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0) {
@@ -595,9 +594,6 @@ if_enqueue(struct ifnet *ifp, struct mbu
}
#endif
- length = m->m_pkthdr.len;
- mflags = m->m_flags;
-
#if NPF > 0
pf_pkt_unlink_state_key(m);
#endif /* NPF > 0 */
@@ -610,11 +606,6 @@ if_enqueue(struct ifnet *ifp, struct mbu
if (error)
return (error);
- ifp->if_opackets++;
- ifp->if_obytes += length;
- if (mflags & M_MCAST)
- ifp->if_omcasts++;
-
if_start(ifp);
return (0);
@@ -1799,10 +1790,12 @@ ifioctl(struct socket *so, u_long cmd, c
ifr->ifr_hardmtu = ifp->if_hardmtu;
break;
- case SIOCGIFDATA:
- error = copyout((caddr_t)&ifp->if_data, ifr->ifr_data,
- sizeof(ifp->if_data));
+ case SIOCGIFDATA: {
+ struct if_data ifdata;
+ if_data(ifp, &ifdata);
+ error = copyout(&ifdata, &ifr->ifr_data, sizeof(ifdata));
break;
+ }
case SIOCSIFFLAGS:
if ((error = suser(p, 0)) != 0)
@@ -2167,6 +2160,33 @@ ifconf(u_long cmd, caddr_t data)
}
ifc->ifc_len -= space;
return (error);
+}
+
+void
+if_data(struct ifnet *ifp, struct if_data *data)
+{
+ struct ifqueue *ifq;
+ uint64_t opackets = 0;
+ uint64_t obytes = 0;
+ uint64_t omcasts = 0;
+ uint64_t oqdrops = 0;
+
+ ifq = &ifp->if_snd;
+
+ mtx_enter(&ifq->ifq_mtx);
+ opackets += ifq->ifq_packets;
+ obytes += ifq->ifq_bytes;
+ oqdrops += ifq->ifq_qdrops;
+ omcasts += ifq->ifq_mcasts;
+ /* ifq->ifq_errors */
+ mtx_leave(&ifq->ifq_mtx);
+
+ *data = ifp->if_data;
+ data->ifi_opackets += opackets;
+ data->ifi_obytes += obytes;
+ data->ifi_oqdrops += oqdrops;
+ data->ifi_omcasts += omcasts;
+ /* ifp->if_data.ifi_oerrors */
}
/*
Index: if.h
===================================================================
RCS file: /cvs/src/sys/net/if.h,v
retrieving revision 1.181
diff -u -p -r1.181 if.h
--- if.h 12 Dec 2016 09:51:30 -0000 1.181
+++ if.h 23 Jan 2017 00:32:53 -0000
@@ -469,6 +469,7 @@ void if_downall(void);
void if_link_state_change(struct ifnet *);
void if_up(struct ifnet *);
int ifconf(u_long, caddr_t);
+void if_data(struct ifnet *, struct if_data *);
void ifinit(void);
int ifioctl(struct socket *, u_long, caddr_t, struct proc *);
int ifpromisc(struct ifnet *, int);
Index: rtsock.c
===================================================================
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.216
diff -u -p -r1.216 rtsock.c
--- rtsock.c 22 Jan 2017 04:31:02 -0000 1.216
+++ rtsock.c 23 Jan 2017 00:32:53 -0000
@@ -1268,7 +1268,7 @@ rt_ifmsg(struct ifnet *ifp)
ifm->ifm_tableid = ifp->if_rdomain;
ifm->ifm_flags = ifp->if_flags;
ifm->ifm_xflags = ifp->if_xflags;
- ifm->ifm_data = ifp->if_data;
+ if_data(ifp, &ifm->ifm_data);
ifm->ifm_addrs = 0;
route_input(m, AF_UNSPEC);
}
@@ -1473,7 +1473,7 @@ sysctl_iflist(int af, struct walkarg *w)
ifm->ifm_index = ifp->if_index;
ifm->ifm_tableid = ifp->if_rdomain;
ifm->ifm_flags = ifp->if_flags;
- ifm->ifm_data = ifp->if_data;
+ if_data(ifp, &ifm->ifm_data);
ifm->ifm_addrs = info.rti_addrs;
error = copyout(ifm, w->w_where, len);
if (error)
