Page MenuHomeFreeBSD
Differential D19804

Use fib4_lookup_nh_ext() in ip_output().
ClosedPublic
Actions

Authored by glebius on Apr 2 2019, 9:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 21, 9:57 AM
Unknown Object (File)
Sun, Apr 20, 3:43 PM
Unknown Object (File)
Sat, Apr 19, 4:23 PM
Unknown Object (File)
Thu, Apr 17, 12:52 PM
Unknown Object (File)
Mon, Apr 14, 10:23 PM
Unknown Object (File)
Mon, Apr 14, 5:56 PM
Unknown Object (File)
Mon, Apr 14, 12:52 PM
Unknown Object (File)
Mon, Apr 14, 12:36 PM
View All Files
Subscribers
ae
imp
melifaro

Details

Reviewers
melifaro
Group Reviewers
network
Commits
rS347375: Existense of PCB route caching doesn't allow us to use new fast route
Summary

Existense of PCB route cachine doesn't allow us to use new fast route
lookup KPI in ip_output() like it is already used in ip_forward().

However, when there is no PCB provided we can use it. Typical scenario
is a DNS server that would use sendto(2) over a not connected UDP socket.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23521
Build 22525: arc lint + arc unit

Event Timeline

glebius created this revision.Apr 2 2019, 9:26 PM
Harbormaster completed remote builds in B23474: Diff 55755.Apr 2 2019, 9:26 PM
melifaro added a subscriber: melifaro.Apr 2 2019, 11:04 PM
melifaro added inline comments.
sys/netinet/ip_output.c
307–311

Nitpick: if we're revisiting this fancy condition, would it be possible to consider replace its occurrence with something like

if (RTE_IS_INVALID(ro->ro_rt) || dst->sin_family != AF_INET ...)

?

glebius added inline comments.Apr 3 2019, 8:20 PM
sys/netinet/ip_output.c
307–311

Do we have RTE_IS_INVALID? But we always first need to check that ro->ro_rt isn't NULL anyway.

glebius updated this revision to Diff 55787.Apr 3 2019, 8:21 PM

More comments & fix a bug with broadcast.

Harbormaster completed remote builds in B23500: Diff 55787.Apr 3 2019, 8:21 PM
glebius updated this revision to Diff 55789.Apr 3 2019, 8:51 PM
  • Copy RTF_HOST to NHF_HOST.
  • Export in_ifaddr as nh_ia. This allows to fix ifa packets/bytes accounting
Herald added a subscriber: ae. · View Herald TranscriptApr 3 2019, 8:51 PM
Harbormaster completed remote builds in B23502: Diff 55789.Apr 3 2019, 8:51 PM
melifaro added inline comments.Apr 4 2019, 1:46 AM
sys/netinet/in_fib.h
46

Would it be possible to move it to a spare field?

sys/netinet/ip_output.c
307–311

No, we don't. I was suggesting adding it as we have these checks repeated at least twice in the ip_output()

443

Seem to always fallback to the in_ifaddr_broadcast() check.

Originally RTF_BROADCAST flag was added 22 years ago in r15652 to avoid calling in_ifaddr_broadcast(). We had route cloning back then, so it was a fair assumption - if the lookup result was a host route (and it almost always was), then we could trust its broadcast flag, as it was set in the in_addroute() after verificaton.

Currently we don't have route cloning and the only way of having RTF_BROADCAST set on the route is to explicitly add host route to the broadcast address:

route add -host 10.0.0.255 -iface vtnet0

assuming 10.0.0.0/24 belongs to vtnet0

Given that, the check here and in line 400 (if (ro->ro_rt->rt_flags & RTF_HOST)) will aways failback to in_ifaddr_broadcast().

Maybe it's worth just eliminating this condition and always check for in_ifaddr_broadcast().

glebius added inline comments.Apr 4 2019, 10:27 PM
sys/netinet/ip_output.c
443

But as long as we allow adding such routes manually, we should support them, shouldn't we?

glebius updated this revision to Diff 55818.Apr 4 2019, 10:28 PM
  • Remove spare, since nh_ia was added.
glebius marked an inline comment as done.Apr 4 2019, 10:28 PM
Harbormaster completed remote builds in B23521: Diff 55818.Apr 4 2019, 10:28 PM
melifaro accepted this revision.Apr 8 2019, 3:22 PM

LGTM. The last one: are you planning to implement the same functionality in ip6_output()? :-)

This revision is now accepted and ready to land.Apr 8 2019, 3:22 PM
glebius added a comment.Apr 10 2019, 12:00 AM

Yes, as a separate change.

Closed by commit rS347375: Existense of PCB route caching doesn't allow us to use new fast route (authored by glebius). · Explain WhyMay 8 2019, 11:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptMay 8 2019, 11:39 PM
ae mentioned this in D34732: Fix ipfw fwd that doesn't work in some cases.Apr 1 2022, 1:52 PM

Revision Contents
Changeset List

PathSize
sys/
net/
1 line
1 line
netinet/
3 lines
7 lines
174 lines

Diff 55818

View Options

sys/net/route.h

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines for (n = 0; n < G_N_EVENTREFS; n++) {
break; break;
} }
} }
if (TAILQ_EMPTY(&g_events)) if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events); wakeup(&g_pending_events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
} }
static int struct g_event *
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap) g_alloc_event(int flag)
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
impAuthorUnsubmitted
Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{ {
struct g_event *ep; KASSERT(flag == M_WAITOK || flag == M_NOWAIT,
("Wrong flag to g_alloc_event"));
return (g_malloc(sizeof(struct g_event), flag | M_ZERO));
}
static void
g_post_event_ep_va(g_event_t *func, void *arg, int wuflag,
struct g_event *ep, va_list ap)
{
void *p; void *p;
u_int n; u_int n;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag | M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag; ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) { for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *); p = va_arg(ap, void *);
if (p == NULL) if (p == NULL)
break; break;
g_trace(G_T_TOPOLOGY, " ref %p", p); g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p; ep->ref[n] = p;
} }
KASSERT(p == NULL, ("Too many references to event")); KASSERT(p == NULL, ("Too many references to event"));
ep->func = func; ep->func = func;
ep->arg = arg; ep->arg = arg;
mtx_lock(&g_eventlock); mtx_lock(&g_eventlock);
TSHOLD("GEOM events"); TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events); TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
wakeup(&g_wait_event); wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags |= TDP_GEOM; curthread->td_pflags |= TDP_GEOM;
thread_lock(curthread); thread_lock(curthread);
curthread->td_flags |= TDF_ASTPENDING; curthread->td_flags |= TDF_ASTPENDING;
thread_unlock(curthread); thread_unlock(curthread);
}
void
g_post_event_ep(g_event_t *func, void *arg, struct g_event *ep, ...)
jhbUnsubmitted
Not Done
Inline Actions

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
impAuthorUnsubmitted
Done
Inline Actions

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
{
va_list ap;
va_start(ap, ep);
g_post_event_ep_va(func, arg, 0, ep, ap);
va_end(ap);
}
static int
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap)
{
struct g_event *ep;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_alloc_event(flag);
if (ep == NULL)
return (ENOMEM);
if (epp != NULL)
*epp = ep;
g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0); return (0);
} }
int int
g_post_event(g_event_t *func, void *arg, int flag, ...) g_post_event(g_event_t *func, void *arg, int flag, ...)
{ {
va_list ap; va_list ap;
int i; int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines
View Options

sys/net/route_var.h

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines for (n = 0; n < G_N_EVENTREFS; n++) {
break; break;
} }
} }
if (TAILQ_EMPTY(&g_events)) if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events); wakeup(&g_pending_events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
} }
static int struct g_event *
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap) g_alloc_event(int flag)
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
impAuthorUnsubmitted
Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{ {
struct g_event *ep; KASSERT(flag == M_WAITOK || flag == M_NOWAIT,
("Wrong flag to g_alloc_event"));
return (g_malloc(sizeof(struct g_event), flag | M_ZERO));
}
static void
g_post_event_ep_va(g_event_t *func, void *arg, int wuflag,
struct g_event *ep, va_list ap)
{
void *p; void *p;
u_int n; u_int n;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag | M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag; ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) { for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *); p = va_arg(ap, void *);
if (p == NULL) if (p == NULL)
break; break;
g_trace(G_T_TOPOLOGY, " ref %p", p); g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p; ep->ref[n] = p;
} }
KASSERT(p == NULL, ("Too many references to event")); KASSERT(p == NULL, ("Too many references to event"));
ep->func = func; ep->func = func;
ep->arg = arg; ep->arg = arg;
mtx_lock(&g_eventlock); mtx_lock(&g_eventlock);
TSHOLD("GEOM events"); TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events); TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
wakeup(&g_wait_event); wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags |= TDP_GEOM; curthread->td_pflags |= TDP_GEOM;
thread_lock(curthread); thread_lock(curthread);
curthread->td_flags |= TDF_ASTPENDING; curthread->td_flags |= TDF_ASTPENDING;
thread_unlock(curthread); thread_unlock(curthread);
}
void
g_post_event_ep(g_event_t *func, void *arg, struct g_event *ep, ...)
jhbUnsubmitted
Not Done
Inline Actions

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
impAuthorUnsubmitted
Done
Inline Actions

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
{
va_list ap;
va_start(ap, ep);
g_post_event_ep_va(func, arg, 0, ep, ap);
va_end(ap);
}
static int
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap)
{
struct g_event *ep;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_alloc_event(flag);
if (ep == NULL)
return (ENOMEM);
if (epp != NULL)
*epp = ep;
g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0); return (0);
} }
int int
g_post_event(g_event_t *func, void *arg, int flag, ...) g_post_event(g_event_t *func, void *arg, int flag, ...)
{ {
va_list ap; va_list ap;
int i; int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines
View Options

sys/netinet/in_fib.h

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines for (n = 0; n < G_N_EVENTREFS; n++) {
break; break;
} }
} }
if (TAILQ_EMPTY(&g_events)) if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events); wakeup(&g_pending_events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
} }
static int struct g_event *
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap) g_alloc_event(int flag)
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
impAuthorUnsubmitted
Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{ {
struct g_event *ep; KASSERT(flag == M_WAITOK || flag == M_NOWAIT,
("Wrong flag to g_alloc_event"));
return (g_malloc(sizeof(struct g_event), flag | M_ZERO));
}
static void
g_post_event_ep_va(g_event_t *func, void *arg, int wuflag,
struct g_event *ep, va_list ap)
{
void *p; void *p;
u_int n; u_int n;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag | M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag; ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) { for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *); p = va_arg(ap, void *);
if (p == NULL) if (p == NULL)
break; break;
g_trace(G_T_TOPOLOGY, " ref %p", p); g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p; ep->ref[n] = p;
} }
KASSERT(p == NULL, ("Too many references to event")); KASSERT(p == NULL, ("Too many references to event"));
ep->func = func; ep->func = func;
ep->arg = arg; ep->arg = arg;
mtx_lock(&g_eventlock); mtx_lock(&g_eventlock);
TSHOLD("GEOM events"); TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events); TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
wakeup(&g_wait_event); wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags |= TDP_GEOM; curthread->td_pflags |= TDP_GEOM;
thread_lock(curthread); thread_lock(curthread);
curthread->td_flags |= TDF_ASTPENDING; curthread->td_flags |= TDF_ASTPENDING;
thread_unlock(curthread); thread_unlock(curthread);
}
void
g_post_event_ep(g_event_t *func, void *arg, struct g_event *ep, ...)
jhbUnsubmitted
Not Done
Inline Actions

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
impAuthorUnsubmitted
Done
Inline Actions

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
{
va_list ap;
va_start(ap, ep);
g_post_event_ep_va(func, arg, 0, ep, ap);
va_end(ap);
}
static int
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap)
{
struct g_event *ep;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_alloc_event(flag);
if (ep == NULL)
return (ENOMEM);
if (epp != NULL)
*epp = ep;
g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0); return (0);
} }
int int
g_post_event(g_event_t *func, void *arg, int flag, ...) g_post_event(g_event_t *func, void *arg, int flag, ...)
{ {
va_list ap; va_list ap;
int i; int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines
View Options

sys/netinet/in_fib.c

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines for (n = 0; n < G_N_EVENTREFS; n++) {
break; break;
} }
} }
if (TAILQ_EMPTY(&g_events)) if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events); wakeup(&g_pending_events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
} }
static int struct g_event *
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap) g_alloc_event(int flag)
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
impAuthorUnsubmitted
Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{ {
struct g_event *ep; KASSERT(flag == M_WAITOK || flag == M_NOWAIT,
("Wrong flag to g_alloc_event"));
return (g_malloc(sizeof(struct g_event), flag | M_ZERO));
}
static void
g_post_event_ep_va(g_event_t *func, void *arg, int wuflag,
struct g_event *ep, va_list ap)
{
void *p; void *p;
u_int n; u_int n;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag | M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag; ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) { for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *); p = va_arg(ap, void *);
if (p == NULL) if (p == NULL)
break; break;
g_trace(G_T_TOPOLOGY, " ref %p", p); g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p; ep->ref[n] = p;
} }
KASSERT(p == NULL, ("Too many references to event")); KASSERT(p == NULL, ("Too many references to event"));
ep->func = func; ep->func = func;
ep->arg = arg; ep->arg = arg;
mtx_lock(&g_eventlock); mtx_lock(&g_eventlock);
TSHOLD("GEOM events"); TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events); TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
wakeup(&g_wait_event); wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags |= TDP_GEOM; curthread->td_pflags |= TDP_GEOM;
thread_lock(curthread); thread_lock(curthread);
curthread->td_flags |= TDF_ASTPENDING; curthread->td_flags |= TDF_ASTPENDING;
thread_unlock(curthread); thread_unlock(curthread);
}
void
g_post_event_ep(g_event_t *func, void *arg, struct g_event *ep, ...)
jhbUnsubmitted
Not Done
Inline Actions

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
impAuthorUnsubmitted
Done
Inline Actions

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
{
va_list ap;
va_start(ap, ep);
g_post_event_ep_va(func, arg, 0, ep, ap);
va_end(ap);
}
static int
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap)
{
struct g_event *ep;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_alloc_event(flag);
if (ep == NULL)
return (ENOMEM);
if (epp != NULL)
*epp = ep;
g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0); return (0);
} }
int int
g_post_event(g_event_t *func, void *arg, int flag, ...) g_post_event(g_event_t *func, void *arg, int flag, ...)
{ {
va_list ap; va_list ap;
int i; int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines
View Options

sys/netinet/ip_output.c

Loading...