Date: Wed, 22 Aug 2012 08:00:48 GMT From: Bruce Evans <bde@FreeBSD.org> To: bde@freebsd.org, freebsd-hackers@freebsd.org, freebsd-net@freebsd.org, mitya@cabletv.dp.ua Subject: Re: Replace bcopy() to update ether_addr Message-ID: <201208220800.q7M80m08021455@ref10-i386.freebsd.org> In-Reply-To: <50347CAB.30309@cabletv.dp.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
mitya wrote:
> 22.08.2012 05:07, Bruce Evans написал:
> >> On Mon, Aug 20, 2012 at 05:46:12PM +0300, Mitya wrote:
> >>> Hi.
> >>> I found some overhead code in /src/sys/net/if_ethersubr.c and
> >>> /src/sys/netgraph/ng_ether.c
> >>>
> >>> It contains strings, like bcopy(src, dst, ETHER_ADDR_LEN);
> >>> When src and dst are "struct ether_addr*", and ETHER_ADDR_LEN equal 6.
> > Only ng_ether.c contains such strings. if_ethersubr.c doesn't exist.
> > if_ether.c exists, but was converted to use memcpy() 17.25 years ago.
Oops, if_ethersubr.c does exist (in net/. if_ether.c is a misnamed file
in netinet/).
> > Summary: use builtin memcpy() for small copies, and don't try hard to
> > otherwise optimize this.
> You are very surprised to learn that bcopy() and memcpy() are used for
> copy "struct in_addr", whose length is equal to 4 bytes ?
> I found this in sys/netinet/if_ether.c. And I think, there is a chance
> to find them in other files.
Since memcpy() is the correct method, us of it is only slightly surprising.
Use of bcopy() is a regression. Bugs are never surprising :-).
> And I found bzero(mtod(m, void *), sizeof(struct in_addr)); in ip_options.c
Speed of options processing isn't important.
Anyway, I dug out some of my old unimportant fixes for some of the
copying pessimizations:
% diff -c2 ./net/if_ethersubr.c~ ./net/if_ethersubr.c
% *** ./net/if_ethersubr.c~ Wed Dec 27 10:49:51 2006
% --- ./net/if_ethersubr.c Wed Dec 27 10:49:52 2006
% ***************
% *** 253,257 ****
% hdrcmplt = 1;
% eh = (struct ether_header *)dst->sa_data;
% ! (void)memcpy(esrc, eh->ether_shost, sizeof (esrc));
% /* FALLTHROUGH */
%
% --- 253,257 ----
% hdrcmplt = 1;
% eh = (struct ether_header *)dst->sa_data;
% ! __builtin_memcpy(esrc, eh->ether_shost, sizeof(esrc));
% /* FALLTHROUGH */
%
Not the right fix (except to fix the style bug (cast to void)).
memcpy() should be used, and then if the builtin is good then it shoukd
be used.
% ***************
% *** 259,263 ****
% loop_copy = 0; /* if this is for us, don't do it */
% eh = (struct ether_header *)dst->sa_data;
% ! (void)memcpy(edst, eh->ether_dhost, sizeof (edst));
% type = eh->ether_type;
% break;
% --- 259,263 ----
% loop_copy = 0; /* if this is for us, don't do it */
% eh = (struct ether_header *)dst->sa_data;
% ! __builtin_memcpy(edst, eh->ether_dhost, sizeof(edst));
% type = eh->ether_type;
% break;
% ***************
% *** 276,288 ****
% senderr(ENOBUFS);
% eh = mtod(m, struct ether_header *);
% ! (void)memcpy(&eh->ether_type, &type,
% ! sizeof(eh->ether_type));
% ! (void)memcpy(eh->ether_dhost, edst, sizeof (edst));
% if (hdrcmplt)
% ! (void)memcpy(eh->ether_shost, esrc,
% ! sizeof(eh->ether_shost));
% else
% ! (void)memcpy(eh->ether_shost, IF_LLADDR(ifp),
% ! sizeof(eh->ether_shost));
%
% /*
% --- 276,287 ----
% senderr(ENOBUFS);
% eh = mtod(m, struct ether_header *);
% ! __builtin_memcpy(&eh->ether_type, &type, sizeof(eh->ether_type));
% ! __builtin_memcpy(eh->ether_dhost, edst, sizeof(edst));
% if (hdrcmplt)
% ! __builtin_memcpy(eh->ether_shost, esrc,
% ! sizeof(eh->ether_shost));
% else
% ! __builtin_memcpy(eh->ether_shost, IF_LLADDR(ifp),
% ! sizeof(eh->ether_shost));
%
% /*
Larger style fixes.
Non-style fixes/breakages are in the z subdir:
% diff -c2 ./net/z/if_ethersubr.c~ ./net/z/if_ethersubr.c
% *** ./net/z/if_ethersubr.c~ Wed Dec 27 10:49:51 2006
% --- ./net/z/if_ethersubr.c Wed Dec 27 20:28:06 2006
% ***************
% *** 191,197 ****
%
% if (m->m_flags & M_BCAST)
% ! bcopy(ifp->if_broadcastaddr, edst, ETHER_ADDR_LEN);
% else
% ! bcopy(ar_tha(ah), edst, ETHER_ADDR_LEN);
%
% }
% --- 191,198 ----
%
% if (m->m_flags & M_BCAST)
% ! __builtin_memcpy(edst, ifp->if_broadcastaddr,
% ! sizeof(edst));
% else
% ! __builtin_memcpy(edst, ar_tha(ah), sizeof(edst));
%
% }
bcopy() can't be replaced by a builtin since it is required to handle
overlapping copies, which I think can't happen here (but I didn't check
this). The builtin shouldn't be hard-coded like this, as above.
% ***************
% *** 214,219 ****
% } else
% type = htons(ETHERTYPE_IPX);
% ! bcopy((caddr_t)&(((struct sockaddr_ipx *)dst)->sipx_addr.x_host),
% ! (caddr_t)edst, sizeof (edst));
% break;
% #endif
% --- 215,221 ----
% } else
% type = htons(ETHERTYPE_IPX);
% ! __buitin_memcpy(edst,
% ! &(((struct sockaddr_ipx *)dst)->sipx_addr.x_host),
% ! sizeof(edst));
% break;
% #endif
As above, plus fix grosser style bugs (use of caddr_t. bcopy() hasn't
take args of type caddr_t for more than 20 years).
% ***************
% *** 238,244 ****
% llc.llc_dsap = llc.llc_ssap = LLC_SNAP_LSAP;
% llc.llc_control = LLC_UI;
% ! bcopy(at_org_code, llc.llc_snap_org_code, sizeof(at_org_code));
% llc.llc_snap_ether_type = htons( ETHERTYPE_AT );
% ! bcopy(&llc, mtod(m, caddr_t), LLC_SNAPFRAMELEN);
% type = htons(m->m_pkthdr.len);
% hlen = LLC_SNAPFRAMELEN + ETHER_HDR_LEN;
% --- 240,247 ----
% llc.llc_dsap = llc.llc_ssap = LLC_SNAP_LSAP;
% llc.llc_control = LLC_UI;
% ! __builtin_memcpy(llc.llc_snap_org_code, at_org_code,
% ! sizeof(at_org_code));
% llc.llc_snap_ether_type = htons( ETHERTYPE_AT );
% ! __builtin_memcpy(mtod(m, caddr_t), &llc, LLC_SNAPFRAMELEN);
% type = htons(m->m_pkthdr.len);
% hlen = LLC_SNAPFRAMELEN + ETHER_HDR_LEN;
% ***************
% *** 253,257 ****
% hdrcmplt = 1;
% eh = (struct ether_header *)dst->sa_data;
% ! (void)memcpy(esrc, eh->ether_shost, sizeof (esrc));
% /* FALLTHROUGH */
%
% --- 256,260 ----
% hdrcmplt = 1;
% eh = (struct ether_header *)dst->sa_data;
% ! __builtin_memcpy(esrc, eh->ether_shost, sizeof(esrc));
% /* FALLTHROUGH */
%
% ***************
% *** 259,263 ****
% loop_copy = 0; /* if this is for us, don't do it */
% eh = (struct ether_header *)dst->sa_data;
% ! (void)memcpy(edst, eh->ether_dhost, sizeof (edst));
% type = eh->ether_type;
% break;
% --- 262,266 ----
% loop_copy = 0; /* if this is for us, don't do it */
% eh = (struct ether_header *)dst->sa_data;
% ! __builtin_memcpy(edst, eh->ether_dhost, sizeof(edst));
% type = eh->ether_type;
% break;
% ***************
% *** 276,288 ****
% senderr(ENOBUFS);
% eh = mtod(m, struct ether_header *);
% ! (void)memcpy(&eh->ether_type, &type,
% ! sizeof(eh->ether_type));
% ! (void)memcpy(eh->ether_dhost, edst, sizeof (edst));
% if (hdrcmplt)
% ! (void)memcpy(eh->ether_shost, esrc,
% ! sizeof(eh->ether_shost));
% else
% ! (void)memcpy(eh->ether_shost, IF_LLADDR(ifp),
% ! sizeof(eh->ether_shost));
%
% /*
% --- 279,290 ----
% senderr(ENOBUFS);
% eh = mtod(m, struct ether_header *);
% ! __builtin_memcpy(&eh->ether_type, &type, sizeof(eh->ether_type));
% ! __builtin_memcpy(eh->ether_dhost, edst, sizeof(edst));
% if (hdrcmplt)
% ! __builtin_memcpy(eh->ether_shost, esrc,
% ! sizeof(eh->ether_shost));
% else
% ! __builtin_memcpy(eh->ether_shost, IF_LLADDR(ifp),
% ! sizeof(eh->ether_shost));
%
% /*
% ***************
% *** 454,459 ****
% }
% if (eh != mtod(m, struct ether_header *))
% ! bcopy(&save_eh, mtod(m, struct ether_header *),
% ! ETHER_HDR_LEN);
% }
% *m0 = m;
% --- 456,461 ----
% }
% if (eh != mtod(m, struct ether_header *))
% ! __builtin_memcpy(mtod(m, struct ether_header *),
% ! &save_eh, sizeof(struct ether_header));
% }
% *m0 = m;
% ***************
% *** 1028,1034 ****
% IF_LLADDR(ifp);
% else {
% ! bcopy((caddr_t) ina->x_host.c_host,
% ! (caddr_t) IF_LLADDR(ifp),
% ! ETHER_ADDR_LEN);
% }
%
% --- 1030,1035 ----
% IF_LLADDR(ifp);
% else {
% ! __builtin_memcpy(IF_LLADDR(ifp),
% ! ina->x_host.c_host, ETHER_ADDR_LEN);
% }
%
% ***************
% *** 1050,1056 ****
% struct sockaddr *sa;
%
% ! sa = (struct sockaddr *) & ifr->ifr_data;
% ! bcopy(IF_LLADDR(ifp),
% ! (caddr_t) sa->sa_data, ETHER_ADDR_LEN);
% }
% break;
% --- 1051,1057 ----
% struct sockaddr *sa;
%
% ! sa = (struct sockaddr *)&ifr->ifr_data;
% ! __builtin_memcpy(sa->sa_data, IF_LLADDR(ifp),
% ! ETHER_ADDR_LEN);
% }
% break;
% ***************
% *** 1208,1212 ****
% KASSERT(m->m_len >= sizeof(struct ether_header),
% ("%s: mbuf not large enough for header", __func__));
% ! bcopy(mtod(m, char *), &vlan, sizeof(struct ether_header));
% vlan.evl_proto = vlan.evl_encap_proto;
% vlan.evl_encap_proto = htons(ETHERTYPE_VLAN);
% --- 1209,1213 ----
% KASSERT(m->m_len >= sizeof(struct ether_header),
% ("%s: mbuf not large enough for header", __func__));
% ! __builtin_memcpy(&vlan, mtod(m, char *), sizeof(&vlan));
% vlan.evl_proto = vlan.evl_encap_proto;
% vlan.evl_encap_proto = htons(ETHERTYPE_VLAN);
This is supposed to fix all the bcopy()'s and (mis)rename all the
memcpy()'s use by a UDP throughput benchmark, and any nearby that
were easy to change.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201208220800.q7M80m08021455>