Page MenuHomeFreeBSD
Differential D42694

sockets: don't malloc/free sockaddr memory on getpeername/getsockname
ClosedPublic
Actions

Authored by glebius on Nov 21 2023, 4:27 AM.
Tags
None
Referenced Files
F113892400: D42694.id130371.diff
Sat, Apr 5, 5:12 AM
Unknown Object (File)
Feb 28 2025, 5:31 PM
Unknown Object (File)
Feb 28 2025, 5:05 PM
Unknown Object (File)
Feb 25 2025, 8:02 AM
Unknown Object (File)
Feb 8 2025, 8:05 PM
Unknown Object (File)
Feb 4 2025, 9:36 PM
Unknown Object (File)
Feb 3 2025, 8:59 AM
Unknown Object (File)
Feb 1 2025, 9:34 PM
View All 56 Files
Subscribers
ae
dchagin
donner
emaste
imp
melifaro
zlei
linuxkpi

Details

Reviewers
kp
melifaro
tuexen
Group Reviewers
transport
network
Linux Emulation
Commits
rG0fac350c54d0: sockets: don't malloc/free sockaddr memory on getpeername/getsockname
Summary

Just like it was done for accept(2) in XXXXXXXXX, use same approach
for two simplier syscalls that return socket addresses. Although,
these two syscalls aren't performance critical, this change generalizes
some code between 3 syscalls trimming code size.

Following example of accept(2), provide VNET-aware and INVARIANT-checking
wrappers sopeeraddr() and sosockaddr() around protosw methods.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 54538
Build 51427: arc lint + arc unit

Event Timeline

glebius created this revision.Nov 21 2023, 4:27 AM
Herald added a reviewer: melifaro. · View Herald TranscriptNov 21 2023, 4:27 AM
Herald added 1 blocking reviewer(s): transport. · View Herald Transcript
Herald added subscribers: linuxkpi, melifaro, donner and 3 others. · View Herald Transcript
glebius requested review of this revision.Nov 21 2023, 4:27 AM
Harbormaster completed remote builds in B54538: Diff 130371.Nov 21 2023, 4:27 AM
dchagin added a subscriber: dchagin.Nov 21 2023, 9:47 AM

just out of curiosity, why to not to add new kernel interfaces aka
kern_getsockname3(struct thread *td, int fd, struct sockaddr_storage *ss)
and below, into the pr methods?

glebius added a comment.Nov 21 2023, 5:28 PM
In D42694#974081, @dchagin wrote:

just out of curiosity, why to not to add new kernel interfaces aka
kern_getsockname3(struct thread *td, int fd, struct sockaddr_storage *ss)
and below, into the pr methods?

Read your comment several times and can't answer anything better than "why to add such a method?" What is the benefit the presence of the method would bring?

tuexen accepted this revision.Nov 30 2023, 3:16 PM
This revision is now accepted and ready to land.Nov 30 2023, 3:16 PM
zlei added a subscriber: zlei.Nov 30 2023, 4:43 PM
zlei added inline comments.
sys/compat/linux/linux_socket.c
1125

len = ss.ss_len;

I'm not understanding this assignment.

The original implementation copyout the MIN(len, sa.sa_len), but with this change ss.ss_len is copyout.

So is the original implementation buggy , or this behavior change is intended ?

1138

The len is copyin from userspace. I guess the sanity check should be preserved.

sys/netgraph/ng_socket.c
514

Maybe we need to move the assignment into the following block ?

	if (pcbp->sockdata->node != NULL) {

	}

So that in case EINVAL the struct sockaddr *sa will keep untouched.

sys/netinet/sctp_usrreq.c
7372

I'm wondering if we need on stack temporary var.

	struct sockaddr_in tmp;
	struct sockaddr_in *sin = &tmp;
....

	*(struct sockaddr_in *)sa = *sin;
sys/netinet6/in6_pcb.c
532

Why the read lock get dropped ?

sys/ofed/drivers/infiniband/ulp/sdp/sdp_main.c
194

With this change and D42635 , sdp_sockaddr() is no longer needed.

Closed by commit rG0fac350c54d0: sockets: don't malloc/free sockaddr memory on getpeername/getsockname (authored by glebius). · Explain WhyNov 30 2023, 5:27 PM
This revision was automatically updated to reflect the committed changes.
glebius added a commit: rG0fac350c54d0: sockets: don't malloc/free sockaddr memory on getpeername/getsockname.
glebius marked an inline comment as done.Nov 30 2023, 10:36 PM
glebius added inline comments.
sys/compat/linux/linux_socket.c
1125

This is intentional. In the previous accept() patch I noted that now we start reporting the actual sockaddr len:

While rewriting accept(2) make 'addrlen' a true in/out parameter, reporting
required length in case if provided length was insufficient. Our manual
page accept(2) and POSIX don't explicitly require that, but one can read
the text as they do. Linux also does that. Update tests accordingly.

And added a test case for that. This change did the same for getsockname()/getpeername(). Probably for Linux emulation this is even more important than for native syscalls.

1138

This check isn't needed. The EINVAL will come from linux_copyout_sockaddr(). Removing this line makes getpeername() be same as getsockname().

sys/netinet/sctp_usrreq.c
7372

Why do we need this?

sys/netinet6/in6_pcb.c
532

These fields of inpcb should be stable in the scope of the syscall. Packets from network can't modify them.

Well, if you write a brain dead program that would run multiple threads doing bind(2)/connect(2) against getsockaddr(2), they would race. But they can also race just in the user context with all the locking present in kernel. The program won't give you consistent results, as sometimes getsockaddr() will run before connect(2) and sometimes after (in the userland). The only difference with such brain dead program would be that now theoretically getsockname() can return an IP address from old connection and port from new one (or vice versa). Do we care about such braindead test or not? My call is we don't.

sys/ofed/drivers/infiniband/ulp/sdp/sdp_main.c
194

Thanks!

zlei added inline comments.Dec 1 2023, 1:47 AM
sys/netinet/sctp_usrreq.c
7372

Why do we need this?

The reason is same with previous comment:

So that in case EINVAL the struct sockaddr *sa will keep untouched

Well in general I personally would prefer not to touch input variables incase ERRORs. I think that is good practice. Also the logic is quite clear and readers will not have to waste time thinking or evaluating why the function errors but my variable get touched, and is that a valid combination ? Will that be harmful in some cases ?.

Anyway the caller should always check return value so touch input variables incase ERROR seems to be good.

glebius marked an inline comment as done.Dec 1 2023, 5:58 AM
glebius added inline comments.
sys/netinet/sctp_usrreq.c
7372

I agree with suggested principle in general, but IMHO it shouldn't be brought down to scope of every function. This particular function expects to be provided with stack allocated sockaddr pointer that is fine to write to. If the function returns an error, the syscall implementation will not copyout(9) the sockaddr on stack to the userland.

Revision Contents
Changeset List

PathSize
sys/
compat/
linux/
45 lines
linuxkpi/
common/
include/
linux/
17 lines
dev/
cxgbe/
iw_cxgbe/
14 lines
hyperv/
hvsock/
4 lines
12 lines
wg/
24 lines
kern/
17 lines
4 lines
34 lines
86 lines
48 lines
net/
12 lines
netgraph/
bluetooth/
include/
3 lines
9 lines
4 lines
4 lines
socket/
8 lines
29 lines
75 lines
47 lines
61 lines
62 lines
28 lines
23 lines
netinet/
6 lines
49 lines
3 lines
7 lines
40 lines
4 lines
netinet6/
1 line
14 lines
10 lines
74 lines
84 lines
netlink/
27 lines
ofed/
drivers/
infiniband/
ulp/
sdp/
36 lines
rpc/
45 lines
9 lines
27 lines
sys/
4 lines
2 lines
6 lines

Diff 130371

View Options

sys/compat/linux/linux_socket.c

Show First 20 Lines • Show All 173 Lines • ▼ Show 20 Linesmlx5e_lro_update_hdr(struct mbuf *mb, struct mlx5_cqe64 *cqe)
} else { } else {
ip6->ip6_hlim = cqe->lro_min_ttl; ip6->ip6_hlim = cqe->lro_min_ttl;
ip6->ip6_plen = cpu_to_be16(tot_len - ip6->ip6_plen = cpu_to_be16(tot_len -
sizeof(struct ip6_hdr)); sizeof(struct ip6_hdr));
} }
/* TODO: handle tcp checksum */ /* TODO: handle tcp checksum */
} }
#define PREC 10
static uint64_t
mlx5e_mbuf_tstmp(struct mlx5e_priv *priv, uint64_t hw_tstmp)
{
struct mlx5e_clbr_point *cp;
uint64_t a1, a2, res;
u_int gen;
do {
cp = &priv->clbr_points[priv->clbr_curr];
gen = atomic_load_acq_int(&cp->clbr_gen);
a1 = (hw_tstmp - cp->clbr_hw_prev) >> PREC;
a2 = (cp->base_curr - cp->base_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is involved in an expressions, the result becomes signed?

hselasky: Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is…
kibAuthorUnsubmitted
Not Done
Inline Actions

There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the result is unsigned as well.

More, the implicit conversion rules result in operations involving at least one unsigned value also performed as unsigned most of the time (but not always, and I am not going to cite the legalese there).

kib: There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the…
res = (a1 * a2) << PREC;
hselaskyUnsubmitted
Done
Inline Actions

Is this multiplication guarded against numerical overflow?

hselasky: Is this multiplication guarded against numerical overflow?
kibAuthorUnsubmitted
Not Done
Inline Actions

Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids loosing the highest significant bits of the result.

kib: Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids…
res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Please add a check for division by zero!

hselasky: Please add a check for division by zero!
kibAuthorUnsubmitted
Not Done
Inline Actions

It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and stops timestamping if they are equal.

After your question, I think that this can be improved by also shifting the difference between the clock readings to simulate the timestamping process more closely. See the new version of the mlx5e_calibration_callout().

kib: It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and…
hselaskyUnsubmitted
Not Done
Inline Actions

Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr - cp->clbr_hw_prev)" and setting "clbr_done" is not atomic!

The callout callback is running unprotected and possibly concurrently with the other piece of code which is accessing these fields. That means clbr_done could initially be >= 2, but later on might be set to zero. Then if the build_rx_mbuf() is running concurrently, this can be an issue.

hselasky: Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr…
kibAuthorUnsubmitted
Not Done
Inline Actions

The algorithm for update is lock-less but it guarantees practically that the calibration data is stable. Calibration callback only updates other clbr_point, the current clbr_point is stable for the whole duration of the calibration interval. After we read current calibration index, we know that it is correct for >= mlx5e_fast_calibration seconds.

So even if clbr_done goes from 2 to 0 due to the driver anxiety over the clock conditions, the current values are still safe.

kib: The algorithm for update is lock-less but it guarantees practically that the calibration data…
hselaskyUnsubmitted
Not Done
Inline Actions

If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf(), then you have no control really how many times mlx5e_calibration_callout() can be called.

  1. if (c->priv->clbr_done >= 2) - thread 2
  1. c->priv->clbr_done = 0 - thread 1
  1. res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) => panic() - thread 2

Further I see that the order of updating "clbr_hw_curr" and "clbr_hw_prev" is not fixed, because volatile is not used in mlx5e_calibration_callout() when setting priv->clbr_curr.

hselasky: If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf()…
kibAuthorUnsubmitted
Not Done
Inline Actions

I do not understand what do you mean by 'how many times'.

The order of updates to clbr_hw_prev/clbr_hw_curr among same clbr_point does not matter because both updates must be visible if update to the cblr_curr is visible.

You might miss that there is more than one calibration points, and switch of the previous clbr_point to the next one, as perceived by the rx routine, is atomic due to the coordination, and switch keeps previous calibration point stable for calibration interval.

kib: I do not understand what do you mean by 'how many times'. The order of updates to…
hselaskyUnsubmitted
Not Done
Inline Actions

I see. No more comments.

hselasky: I see. No more comments.
res += cp->base_prev;
atomic_thread_fence_acq();
} while (gen == 0 || gen != cp->clbr_gen);
return (res);
}
static inline void static inline void
mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe, mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe,
struct mlx5e_rq *rq, struct mbuf *mb, struct mlx5e_rq *rq, struct mbuf *mb,
u32 cqe_bcnt) u32 cqe_bcnt)
{ {
struct ifnet *ifp = rq->ifp; struct ifnet *ifp = rq->ifp;
struct mlx5e_channel *c;
int lro_num_seg; /* HW LRO session aggregated packets counter */ int lro_num_seg; /* HW LRO session aggregated packets counter */
uint64_t tstmp;
lro_num_seg = be32_to_cpu(cqe->srqn) >> 24; lro_num_seg = be32_to_cpu(cqe->srqn) >> 24;
if (lro_num_seg > 1) { if (lro_num_seg > 1) {
mlx5e_lro_update_hdr(mb, cqe); mlx5e_lro_update_hdr(mb, cqe);
rq->stats.lro_packets++; rq->stats.lro_packets++;
rq->stats.lro_bytes += cqe_bcnt; rq->stats.lro_bytes += cqe_bcnt;
} }
▲ Show 20 Lines • Show All 47 Lines • ▼ Show 20 Lines if (likely(ifp->if_capenable & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)) &&
mb->m_pkthdr.csum_data = htons(0xffff); mb->m_pkthdr.csum_data = htons(0xffff);
} else { } else {
rq->stats.csum_none++; rq->stats.csum_none++;
} }
if (cqe_has_vlan(cqe)) { if (cqe_has_vlan(cqe)) {
mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info); mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info);
mb->m_flags |= M_VLANTAG; mb->m_flags |= M_VLANTAG;
}
c = container_of(rq, struct mlx5e_channel, rq);
if (c->priv->clbr_done >= 2) {
tstmp = mlx5e_mbuf_tstmp(c->priv, be64_to_cpu(cqe->timestamp));
if ((tstmp & MLX5_CQE_TSTMP_PTP) != 0) {
/*
* Timestamp was taken on the packet entrance,
* instead of the cqe generation.
*/
tstmp &= ~MLX5_CQE_TSTMP_PTP;
mb->m_flags |= M_TSTMP_HPREC;
}
mb->m_pkthdr.rcv_tstmp = tstmp;
mb->m_flags |= M_TSTMP;
} }
} }
static inline void static inline void
mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data) mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data)
{ {
memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)), memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)),
sizeof(struct mlx5_cqe64)); sizeof(struct mlx5_cqe64));
▲ Show 20 Lines • Show All 177 Lines • Show Last 20 Lines
View Options

sys/compat/linuxkpi/common/include/linux/net.h

Show First 20 Lines • Show All 173 Lines • ▼ Show 20 Linesmlx5e_lro_update_hdr(struct mbuf *mb, struct mlx5_cqe64 *cqe)
} else { } else {
ip6->ip6_hlim = cqe->lro_min_ttl; ip6->ip6_hlim = cqe->lro_min_ttl;
ip6->ip6_plen = cpu_to_be16(tot_len - ip6->ip6_plen = cpu_to_be16(tot_len -
sizeof(struct ip6_hdr)); sizeof(struct ip6_hdr));
} }
/* TODO: handle tcp checksum */ /* TODO: handle tcp checksum */
} }
#define PREC 10
static uint64_t
mlx5e_mbuf_tstmp(struct mlx5e_priv *priv, uint64_t hw_tstmp)
{
struct mlx5e_clbr_point *cp;
uint64_t a1, a2, res;
u_int gen;
do {
cp = &priv->clbr_points[priv->clbr_curr];
gen = atomic_load_acq_int(&cp->clbr_gen);
a1 = (hw_tstmp - cp->clbr_hw_prev) >> PREC;
a2 = (cp->base_curr - cp->base_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is involved in an expressions, the result becomes signed?

hselasky: Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is…
kibAuthorUnsubmitted
Not Done
Inline Actions

There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the result is unsigned as well.

More, the implicit conversion rules result in operations involving at least one unsigned value also performed as unsigned most of the time (but not always, and I am not going to cite the legalese there).

kib: There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the…
res = (a1 * a2) << PREC;
hselaskyUnsubmitted
Done
Inline Actions

Is this multiplication guarded against numerical overflow?

hselasky: Is this multiplication guarded against numerical overflow?
kibAuthorUnsubmitted
Not Done
Inline Actions

Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids loosing the highest significant bits of the result.

kib: Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids…
res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Please add a check for division by zero!

hselasky: Please add a check for division by zero!
kibAuthorUnsubmitted
Not Done
Inline Actions

It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and stops timestamping if they are equal.

After your question, I think that this can be improved by also shifting the difference between the clock readings to simulate the timestamping process more closely. See the new version of the mlx5e_calibration_callout().

kib: It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and…
hselaskyUnsubmitted
Not Done
Inline Actions

Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr - cp->clbr_hw_prev)" and setting "clbr_done" is not atomic!

The callout callback is running unprotected and possibly concurrently with the other piece of code which is accessing these fields. That means clbr_done could initially be >= 2, but later on might be set to zero. Then if the build_rx_mbuf() is running concurrently, this can be an issue.

hselasky: Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr…
kibAuthorUnsubmitted
Not Done
Inline Actions

The algorithm for update is lock-less but it guarantees practically that the calibration data is stable. Calibration callback only updates other clbr_point, the current clbr_point is stable for the whole duration of the calibration interval. After we read current calibration index, we know that it is correct for >= mlx5e_fast_calibration seconds.

So even if clbr_done goes from 2 to 0 due to the driver anxiety over the clock conditions, the current values are still safe.

kib: The algorithm for update is lock-less but it guarantees practically that the calibration data…
hselaskyUnsubmitted
Not Done
Inline Actions

If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf(), then you have no control really how many times mlx5e_calibration_callout() can be called.

  1. if (c->priv->clbr_done >= 2) - thread 2
  1. c->priv->clbr_done = 0 - thread 1
  1. res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) => panic() - thread 2

Further I see that the order of updating "clbr_hw_curr" and "clbr_hw_prev" is not fixed, because volatile is not used in mlx5e_calibration_callout() when setting priv->clbr_curr.

hselasky: If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf()…
kibAuthorUnsubmitted
Not Done
Inline Actions

I do not understand what do you mean by 'how many times'.

The order of updates to clbr_hw_prev/clbr_hw_curr among same clbr_point does not matter because both updates must be visible if update to the cblr_curr is visible.

You might miss that there is more than one calibration points, and switch of the previous clbr_point to the next one, as perceived by the rx routine, is atomic due to the coordination, and switch keeps previous calibration point stable for calibration interval.

kib: I do not understand what do you mean by 'how many times'. The order of updates to…
hselaskyUnsubmitted
Not Done
Inline Actions

I see. No more comments.

hselasky: I see. No more comments.
res += cp->base_prev;
atomic_thread_fence_acq();
} while (gen == 0 || gen != cp->clbr_gen);
return (res);
}
static inline void static inline void
mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe, mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe,
struct mlx5e_rq *rq, struct mbuf *mb, struct mlx5e_rq *rq, struct mbuf *mb,
u32 cqe_bcnt) u32 cqe_bcnt)
{ {
struct ifnet *ifp = rq->ifp; struct ifnet *ifp = rq->ifp;
struct mlx5e_channel *c;
int lro_num_seg; /* HW LRO session aggregated packets counter */ int lro_num_seg; /* HW LRO session aggregated packets counter */
uint64_t tstmp;
lro_num_seg = be32_to_cpu(cqe->srqn) >> 24; lro_num_seg = be32_to_cpu(cqe->srqn) >> 24;
if (lro_num_seg > 1) { if (lro_num_seg > 1) {
mlx5e_lro_update_hdr(mb, cqe); mlx5e_lro_update_hdr(mb, cqe);
rq->stats.lro_packets++; rq->stats.lro_packets++;
rq->stats.lro_bytes += cqe_bcnt; rq->stats.lro_bytes += cqe_bcnt;
} }
▲ Show 20 Lines • Show All 47 Lines • ▼ Show 20 Lines if (likely(ifp->if_capenable & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)) &&
mb->m_pkthdr.csum_data = htons(0xffff); mb->m_pkthdr.csum_data = htons(0xffff);
} else { } else {
rq->stats.csum_none++; rq->stats.csum_none++;
} }
if (cqe_has_vlan(cqe)) { if (cqe_has_vlan(cqe)) {
mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info); mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info);
mb->m_flags |= M_VLANTAG; mb->m_flags |= M_VLANTAG;
}
c = container_of(rq, struct mlx5e_channel, rq);
if (c->priv->clbr_done >= 2) {
tstmp = mlx5e_mbuf_tstmp(c->priv, be64_to_cpu(cqe->timestamp));
if ((tstmp & MLX5_CQE_TSTMP_PTP) != 0) {
/*
* Timestamp was taken on the packet entrance,
* instead of the cqe generation.
*/
tstmp &= ~MLX5_CQE_TSTMP_PTP;
mb->m_flags |= M_TSTMP_HPREC;
}
mb->m_pkthdr.rcv_tstmp = tstmp;
mb->m_flags |= M_TSTMP;
} }
} }
static inline void static inline void
mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data) mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data)
{ {
memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)), memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)),
sizeof(struct mlx5_cqe64)); sizeof(struct mlx5_cqe64));
▲ Show 20 Lines • Show All 177 Lines • Show Last 20 Lines
View Options

sys/dev/cxgbe/iw_cxgbe/cm.c

Show First 20 Lines • Show All 173 Lines • ▼ Show 20 Linesmlx5e_lro_update_hdr(struct mbuf *mb, struct mlx5_cqe64 *cqe)
} else { } else {
ip6->ip6_hlim = cqe->lro_min_ttl; ip6->ip6_hlim = cqe->lro_min_ttl;
ip6->ip6_plen = cpu_to_be16(tot_len - ip6->ip6_plen = cpu_to_be16(tot_len -
sizeof(struct ip6_hdr)); sizeof(struct ip6_hdr));
} }
/* TODO: handle tcp checksum */ /* TODO: handle tcp checksum */
} }
#define PREC 10
static uint64_t
mlx5e_mbuf_tstmp(struct mlx5e_priv *priv, uint64_t hw_tstmp)
{
struct mlx5e_clbr_point *cp;
uint64_t a1, a2, res;
u_int gen;
do {
cp = &priv->clbr_points[priv->clbr_curr];
gen = atomic_load_acq_int(&cp->clbr_gen);
a1 = (hw_tstmp - cp->clbr_hw_prev) >> PREC;
a2 = (cp->base_curr - cp->base_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is involved in an expressions, the result becomes signed?

hselasky: Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is…
kibAuthorUnsubmitted
Not Done
Inline Actions

There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the result is unsigned as well.

More, the implicit conversion rules result in operations involving at least one unsigned value also performed as unsigned most of the time (but not always, and I am not going to cite the legalese there).

kib: There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the…
res = (a1 * a2) << PREC;
hselaskyUnsubmitted
Done
Inline Actions

Is this multiplication guarded against numerical overflow?

hselasky: Is this multiplication guarded against numerical overflow?
kibAuthorUnsubmitted
Not Done
Inline Actions

Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids loosing the highest significant bits of the result.

kib: Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids…
res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Please add a check for division by zero!

hselasky: Please add a check for division by zero!
kibAuthorUnsubmitted
Not Done
Inline Actions

It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and stops timestamping if they are equal.

After your question, I think that this can be improved by also shifting the difference between the clock readings to simulate the timestamping process more closely. See the new version of the mlx5e_calibration_callout().

kib: It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and…
hselaskyUnsubmitted
Not Done
Inline Actions

Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr - cp->clbr_hw_prev)" and setting "clbr_done" is not atomic!

The callout callback is running unprotected and possibly concurrently with the other piece of code which is accessing these fields. That means clbr_done could initially be >= 2, but later on might be set to zero. Then if the build_rx_mbuf() is running concurrently, this can be an issue.

hselasky: Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr…
kibAuthorUnsubmitted
Not Done
Inline Actions

The algorithm for update is lock-less but it guarantees practically that the calibration data is stable. Calibration callback only updates other clbr_point, the current clbr_point is stable for the whole duration of the calibration interval. After we read current calibration index, we know that it is correct for >= mlx5e_fast_calibration seconds.

So even if clbr_done goes from 2 to 0 due to the driver anxiety over the clock conditions, the current values are still safe.

kib: The algorithm for update is lock-less but it guarantees practically that the calibration data…
hselaskyUnsubmitted
Not Done
Inline Actions

If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf(), then you have no control really how many times mlx5e_calibration_callout() can be called.

  1. if (c->priv->clbr_done >= 2) - thread 2
  1. c->priv->clbr_done = 0 - thread 1
  1. res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) => panic() - thread 2

Further I see that the order of updating "clbr_hw_curr" and "clbr_hw_prev" is not fixed, because volatile is not used in mlx5e_calibration_callout() when setting priv->clbr_curr.

hselasky: If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf()…
kibAuthorUnsubmitted
Not Done
Inline Actions

I do not understand what do you mean by 'how many times'.

The order of updates to clbr_hw_prev/clbr_hw_curr among same clbr_point does not matter because both updates must be visible if update to the cblr_curr is visible.

You might miss that there is more than one calibration points, and switch of the previous clbr_point to the next one, as perceived by the rx routine, is atomic due to the coordination, and switch keeps previous calibration point stable for calibration interval.

kib: I do not understand what do you mean by 'how many times'. The order of updates to…
hselaskyUnsubmitted
Not Done
Inline Actions

I see. No more comments.

hselasky: I see. No more comments.
res += cp->base_prev;
atomic_thread_fence_acq();
} while (gen == 0 || gen != cp->clbr_gen);
return (res);
}
static inline void static inline void
mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe, mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe,
struct mlx5e_rq *rq, struct mbuf *mb, struct mlx5e_rq *rq, struct mbuf *mb,
u32 cqe_bcnt) u32 cqe_bcnt)
{ {
struct ifnet *ifp = rq->ifp; struct ifnet *ifp = rq->ifp;
struct mlx5e_channel *c;
int lro_num_seg; /* HW LRO session aggregated packets counter */ int lro_num_seg; /* HW LRO session aggregated packets counter */
uint64_t tstmp;
lro_num_seg = be32_to_cpu(cqe->srqn) >> 24; lro_num_seg = be32_to_cpu(cqe->srqn) >> 24;
if (lro_num_seg > 1) { if (lro_num_seg > 1) {
mlx5e_lro_update_hdr(mb, cqe); mlx5e_lro_update_hdr(mb, cqe);
rq->stats.lro_packets++; rq->stats.lro_packets++;
rq->stats.lro_bytes += cqe_bcnt; rq->stats.lro_bytes += cqe_bcnt;
} }
▲ Show 20 Lines • Show All 47 Lines • ▼ Show 20 Lines if (likely(ifp->if_capenable & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)) &&
mb->m_pkthdr.csum_data = htons(0xffff); mb->m_pkthdr.csum_data = htons(0xffff);
} else { } else {
rq->stats.csum_none++; rq->stats.csum_none++;
} }
if (cqe_has_vlan(cqe)) { if (cqe_has_vlan(cqe)) {
mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info); mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info);
mb->m_flags |= M_VLANTAG; mb->m_flags |= M_VLANTAG;
}
c = container_of(rq, struct mlx5e_channel, rq);
if (c->priv->clbr_done >= 2) {
tstmp = mlx5e_mbuf_tstmp(c->priv, be64_to_cpu(cqe->timestamp));
if ((tstmp & MLX5_CQE_TSTMP_PTP) != 0) {
/*
* Timestamp was taken on the packet entrance,
* instead of the cqe generation.
*/
tstmp &= ~MLX5_CQE_TSTMP_PTP;
mb->m_flags |= M_TSTMP_HPREC;
}
mb->m_pkthdr.rcv_tstmp = tstmp;
mb->m_flags |= M_TSTMP;
} }
} }
static inline void static inline void
mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data) mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data)
{ {
memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)), memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)),
sizeof(struct mlx5_cqe64)); sizeof(struct mlx5_cqe64));
▲ Show 20 Lines • Show All 177 Lines • Show Last 20 Lines
View Options

sys/dev/hyperv/hvsock/hv_sock.h

Show First 20 Lines • Show All 173 Lines • ▼ Show 20 Linesmlx5e_lro_update_hdr(struct mbuf *mb, struct mlx5_cqe64 *cqe)
} else { } else {
ip6->ip6_hlim = cqe->lro_min_ttl; ip6->ip6_hlim = cqe->lro_min_ttl;
ip6->ip6_plen = cpu_to_be16(tot_len - ip6->ip6_plen = cpu_to_be16(tot_len -
sizeof(struct ip6_hdr)); sizeof(struct ip6_hdr));
} }
/* TODO: handle tcp checksum */ /* TODO: handle tcp checksum */
} }
#define PREC 10
static uint64_t
mlx5e_mbuf_tstmp(struct mlx5e_priv *priv, uint64_t hw_tstmp)
{
struct mlx5e_clbr_point *cp;
uint64_t a1, a2, res;
u_int gen;
do {
cp = &priv->clbr_points[priv->clbr_curr];
gen = atomic_load_acq_int(&cp->clbr_gen);
a1 = (hw_tstmp - cp->clbr_hw_prev) >> PREC;
a2 = (cp->base_curr - cp->base_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is involved in an expressions, the result becomes signed?

hselasky: Are you doing signed or unsigned shift here? Isn't there a compiler rule that when "-" minus is…
kibAuthorUnsubmitted
Not Done
Inline Actions

There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the result is unsigned as well.

More, the implicit conversion rules result in operations involving at least one unsigned value also performed as unsigned most of the time (but not always, and I am not going to cite the legalese there).

kib: There is no such 'rule'. Arithmetic between unsigned data is performed as unsigned and the…
res = (a1 * a2) << PREC;
hselaskyUnsubmitted
Done
Inline Actions

Is this multiplication guarded against numerical overflow?

hselasky: Is this multiplication guarded against numerical overflow?
kibAuthorUnsubmitted
Not Done
Inline Actions

Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids loosing the highest significant bits of the result.

kib: Yes, this is exactly why the PREC stuff is there. It reduces the precision but also avoids…
res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) >> PREC;
hselaskyUnsubmitted
Done
Inline Actions

Please add a check for division by zero!

hselasky: Please add a check for division by zero!
kibAuthorUnsubmitted
Not Done
Inline Actions

It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and stops timestamping if they are equal.

After your question, I think that this can be improved by also shifting the difference between the clock readings to simulate the timestamping process more closely. See the new version of the mlx5e_calibration_callout().

kib: It is already guarded, the calibration callback compares clbr_hw_curr and clbr_hw_prev and…
hselaskyUnsubmitted
Not Done
Inline Actions

Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr - cp->clbr_hw_prev)" and setting "clbr_done" is not atomic!

The callout callback is running unprotected and possibly concurrently with the other piece of code which is accessing these fields. That means clbr_done could initially be >= 2, but later on might be set to zero. Then if the build_rx_mbuf() is running concurrently, this can be an issue.

hselasky: Right, but remember that checking "if (c->priv->clbr_done >= 2)" and reading "(cp->clbr_hw_curr…
kibAuthorUnsubmitted
Not Done
Inline Actions

The algorithm for update is lock-less but it guarantees practically that the calibration data is stable. Calibration callback only updates other clbr_point, the current clbr_point is stable for the whole duration of the calibration interval. After we read current calibration index, we know that it is correct for >= mlx5e_fast_calibration seconds.

So even if clbr_done goes from 2 to 0 due to the driver anxiety over the clock conditions, the current values are still safe.

kib: The algorithm for update is lock-less but it guarantees practically that the calibration data…
hselaskyUnsubmitted
Not Done
Inline Actions

If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf(), then you have no control really how many times mlx5e_calibration_callout() can be called.

  1. if (c->priv->clbr_done >= 2) - thread 2
  1. c->priv->clbr_done = 0 - thread 1
  1. res /= (cp->clbr_hw_curr - cp->clbr_hw_prev) => panic() - thread 2

Further I see that the order of updating "clbr_hw_curr" and "clbr_hw_prev" is not fixed, because volatile is not used in mlx5e_calibration_callout() when setting priv->clbr_curr.

hselasky: If thread 1 enters, mlx5e_calibration_callout(), while thread 2 enters mlx5e_build_rx_mbuf()…
kibAuthorUnsubmitted
Not Done
Inline Actions

I do not understand what do you mean by 'how many times'.

The order of updates to clbr_hw_prev/clbr_hw_curr among same clbr_point does not matter because both updates must be visible if update to the cblr_curr is visible.

You might miss that there is more than one calibration points, and switch of the previous clbr_point to the next one, as perceived by the rx routine, is atomic due to the coordination, and switch keeps previous calibration point stable for calibration interval.

kib: I do not understand what do you mean by 'how many times'. The order of updates to…
hselaskyUnsubmitted
Not Done
Inline Actions

I see. No more comments.

hselasky: I see. No more comments.
res += cp->base_prev;
atomic_thread_fence_acq();
} while (gen == 0 || gen != cp->clbr_gen);
return (res);
}
static inline void static inline void
mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe, mlx5e_build_rx_mbuf(struct mlx5_cqe64 *cqe,
struct mlx5e_rq *rq, struct mbuf *mb, struct mlx5e_rq *rq, struct mbuf *mb,
u32 cqe_bcnt) u32 cqe_bcnt)
{ {
struct ifnet *ifp = rq->ifp; struct ifnet *ifp = rq->ifp;
struct mlx5e_channel *c;
int lro_num_seg; /* HW LRO session aggregated packets counter */ int lro_num_seg; /* HW LRO session aggregated packets counter */
uint64_t tstmp;
lro_num_seg = be32_to_cpu(cqe->srqn) >> 24; lro_num_seg = be32_to_cpu(cqe->srqn) >> 24;
if (lro_num_seg > 1) { if (lro_num_seg > 1) {
mlx5e_lro_update_hdr(mb, cqe); mlx5e_lro_update_hdr(mb, cqe);
rq->stats.lro_packets++; rq->stats.lro_packets++;
rq->stats.lro_bytes += cqe_bcnt; rq->stats.lro_bytes += cqe_bcnt;
} }
▲ Show 20 Lines • Show All 47 Lines • ▼ Show 20 Lines if (likely(ifp->if_capenable & (IFCAP_RXCSUM | IFCAP_RXCSUM_IPV6)) &&
mb->m_pkthdr.csum_data = htons(0xffff); mb->m_pkthdr.csum_data = htons(0xffff);
} else { } else {
rq->stats.csum_none++; rq->stats.csum_none++;
} }
if (cqe_has_vlan(cqe)) { if (cqe_has_vlan(cqe)) {
mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info); mb->m_pkthdr.ether_vtag = be16_to_cpu(cqe->vlan_info);
mb->m_flags |= M_VLANTAG; mb->m_flags |= M_VLANTAG;
}
c = container_of(rq, struct mlx5e_channel, rq);
if (c->priv->clbr_done >= 2) {
tstmp = mlx5e_mbuf_tstmp(c->priv, be64_to_cpu(cqe->timestamp));
if ((tstmp & MLX5_CQE_TSTMP_PTP) != 0) {
/*
* Timestamp was taken on the packet entrance,
* instead of the cqe generation.
*/
tstmp &= ~MLX5_CQE_TSTMP_PTP;
mb->m_flags |= M_TSTMP_HPREC;
}
mb->m_pkthdr.rcv_tstmp = tstmp;
mb->m_flags |= M_TSTMP;
} }
} }
static inline void static inline void
mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data) mlx5e_read_cqe_slot(struct mlx5e_cq *cq, u32 cc, void *data)
{ {
memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)), memcpy(data, mlx5_cqwq_get_wqe(&cq->wq, (cc & cq->wq.sz_m1)),
sizeof(struct mlx5_cqe64)); sizeof(struct mlx5_cqe64));
▲ Show 20 Lines • Show All 177 Lines • Show Last 20 Lines
View Options

sys/dev/hyperv/hvsock/hv_sock.c

Loading...
View Options

sys/dev/wg/if_wg.c

Loading...
View Options

sys/kern/sys_socket.c

Loading...
View Options

sys/kern/uipc_domain.c

Loading...
View Options

sys/kern/uipc_socket.c

Loading...
View Options

sys/kern/uipc_syscalls.c

Loading...
View Options

sys/kern/uipc_usrreq.c

Loading...
View Options

sys/net/if_ovpn.c

Loading...
View Options

sys/netgraph/bluetooth/include/ng_btsocket_hci_raw.h

Loading...
View Options

sys/netgraph/bluetooth/include/ng_btsocket_l2cap.h

Loading...
View Options

sys/netgraph/bluetooth/include/ng_btsocket_rfcomm.h

Loading...
View Options

sys/netgraph/bluetooth/include/ng_btsocket_sco.h

Loading...
View Options

sys/netgraph/bluetooth/socket/ng_btsocket.c

Loading...
View Options

sys/netgraph/bluetooth/socket/ng_btsocket_hci_raw.c

Loading...
View Options

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap.c

Loading...
View Options

sys/netgraph/bluetooth/socket/ng_btsocket_l2cap_raw.c

Loading...
View Options

sys/netgraph/bluetooth/socket/ng_btsocket_rfcomm.c

Loading...
View Options

sys/netgraph/bluetooth/socket/ng_btsocket_sco.c

Loading...
View Options

sys/netgraph/ng_ksocket.c

Loading...
View Options

sys/netgraph/ng_socket.c

Loading...
View Options

sys/netinet/in_pcb.h

Loading...
View Options

sys/netinet/in_pcb.c

Loading...
View Options

sys/netinet/sctp_os.h

Loading...
View Options

sys/netinet/sctp_os_bsd.h

Loading...
View Options

sys/netinet/sctp_usrreq.c

Loading...
View Options

sys/netinet/sctp_var.h

Loading...
View Options

sys/netinet6/in6.h

Loading...
View Options

sys/netinet6/in6.c

Loading...
View Options

sys/netinet6/in6_pcb.h

Loading...
View Options

sys/netinet6/in6_pcb.c

Loading...
View Options

sys/netinet6/sctp6_usrreq.c

Loading...
View Options

sys/netlink/netlink_domain.c

Loading...
View Options

sys/ofed/drivers/infiniband/ulp/sdp/sdp_main.c

Loading...
View Options

sys/rpc/rpc_generic.c

Loading...
View Options

sys/rpc/svc_dg.c

Loading...
View Options

sys/rpc/svc_vc.c

Loading...
View Options

sys/sys/protosw.h

Loading...
View Options

sys/sys/socketvar.h

Loading...
View Options

sys/sys/syscallsubr.h

Loading...