From owner-freebsd-hackers@FreeBSD.ORG Wed Aug 22 08:00:49 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 75BE0106564A; Wed, 22 Aug 2012 08:00:49 +0000 (UTC) (envelope-from bde@FreeBSD.org) Received: from ref10-i386.freebsd.org (unknown [IPv6:2001:4f8:fff6::5e]) by mx1.freebsd.org (Postfix) with ESMTP id 5FE388FC08; Wed, 22 Aug 2012 08:00:49 +0000 (UTC) Received: from ref10-i386.freebsd.org (localhost [127.0.0.1]) by ref10-i386.freebsd.org (8.14.5/8.14.5) with ESMTP id q7M80n3q021456; Wed, 22 Aug 2012 08:00:49 GMT (envelope-from bde@ref10-i386.freebsd.org) Received: (from bde@localhost) by ref10-i386.freebsd.org (8.14.5/8.14.5/Submit) id q7M80m08021455; Wed, 22 Aug 2012 08:00:48 GMT (envelope-from bde) Date: Wed, 22 Aug 2012 08:00:48 GMT From: Bruce Evans Message-Id: <201208220800.q7M80m08021455@ref10-i386.freebsd.org> To: bde@freebsd.org, freebsd-hackers@freebsd.org, freebsd-net@freebsd.org, mitya@cabletv.dp.ua In-Reply-To: <50347CAB.30309@cabletv.dp.ua> Cc: Subject: Re: Replace bcopy() to update ether_addr X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Aug 2012 08:00:49 -0000 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