Skip site navigation (1)Skip section navigation (2)
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>