Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On Thu, Jan 16, 2020 at 12:06:17PM -0800, Cy Schubert wrote: C> On January 15, 2020 2:38:05 PM PST, Gleb Smirnoff wrote: C> >On Wed, Jan 15, 2020 at 09:44:53AM -1000, Jeff Roberson wrote: C> >J> On Wed, 15 Jan 2020, Gleb Smirnoff wrote: C> >J> C> >J> > Author: glebius C> >J> > Date: Wed Jan 15 06:05:20 2020 C> >J> > New Revision: 356755 C> >J> > URL: https://svnweb.freebsd.org/changeset/base/356755 C> >J> > C> >J> > Log: C> >J> > Introduce NET_EPOCH_CALL() macro and use it everywhere where we C> >free C> >J> > data based on the network epoch. The macro reverses the C> >argument C> >J> > order of epoch_call(9) - first function, then its argument. NFC C> >J> C> >J> Is there some practical impact of changing the argument order or C> >does it C> >J> just seem more natural to you? C> > C> >It is just more natural. I'm suggesting to change prototype of C> >epoch_call() C> >to the same order as well. C> C> Yes. C> C> Are there any ports that might be affected, such as kms-drm or virtualbox-ose-kmod? I'm not aware of any software outside /usr/src/sys that uses epoch. -- Gleb Smirnoff ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On January 15, 2020 2:38:05 PM PST, Gleb Smirnoff wrote: >On Wed, Jan 15, 2020 at 09:44:53AM -1000, Jeff Roberson wrote: >J> On Wed, 15 Jan 2020, Gleb Smirnoff wrote: >J> >J> > Author: glebius >J> > Date: Wed Jan 15 06:05:20 2020 >J> > New Revision: 356755 >J> > URL: https://svnweb.freebsd.org/changeset/base/356755 >J> > >J> > Log: >J> > Introduce NET_EPOCH_CALL() macro and use it everywhere where we >free >J> > data based on the network epoch. The macro reverses the >argument >J> > order of epoch_call(9) - first function, then its argument. NFC >J> >J> Is there some practical impact of changing the argument order or >does it >J> just seem more natural to you? > >It is just more natural. I'm suggesting to change prototype of >epoch_call() >to the same order as well. Yes. Are there any ports that might be affected, such as kms-drm or virtualbox-ose-kmod? -- Pardon the typos and autocorrect, small keyboard in use. Cy Schubert FreeBSD UNIX: Web: https://www.FreeBSD.org The need of the many outweighs the greed of the few. Sent from my Android device with K-9 Mail. Please excuse my brevity. ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On 1/15/20 2:38 PM, Gleb Smirnoff wrote: > On Wed, Jan 15, 2020 at 09:44:53AM -1000, Jeff Roberson wrote: > J> On Wed, 15 Jan 2020, Gleb Smirnoff wrote: > J> > J> > Author: glebius > J> > Date: Wed Jan 15 06:05:20 2020 > J> > New Revision: 356755 > J> > URL: https://svnweb.freebsd.org/changeset/base/356755 > J> > > J> > Log: > J> > Introduce NET_EPOCH_CALL() macro and use it everywhere where we free > J> > data based on the network epoch. The macro reverses the argument > J> > order of epoch_call(9) - first function, then its argument. NFC > J> > J> Is there some practical impact of changing the argument order or does it > J> just seem more natural to you? > > It is just more natural. I'm suggesting to change prototype of epoch_call() > to the same order as well. +1 for fn, arg. -- John Baldwin ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On Wed, Jan 15, 2020 at 12:36 PM Warner Losh wrote: > Everywhere else in the system, it's fnp, argp for ordering the args... You'd > be hard pressed to find exceptions to this rule. Since this is a new > interface, we should fix it now while we still can :) qsort_r() is one exception (and that ordering is a mistake in many ways). +1 to Gleb's proposed new world order. ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On Wed, Jan 15, 2020 at 09:44:53AM -1000, Jeff Roberson wrote: J> On Wed, 15 Jan 2020, Gleb Smirnoff wrote: J> J> > Author: glebius J> > Date: Wed Jan 15 06:05:20 2020 J> > New Revision: 356755 J> > URL: https://svnweb.freebsd.org/changeset/base/356755 J> > J> > Log: J> > Introduce NET_EPOCH_CALL() macro and use it everywhere where we free J> > data based on the network epoch. The macro reverses the argument J> > order of epoch_call(9) - first function, then its argument. NFC J> J> Is there some practical impact of changing the argument order or does it J> just seem more natural to you? It is just more natural. I'm suggesting to change prototype of epoch_call() to the same order as well. -- Gleb Smirnoff ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On Wed, Jan 15, 2020 at 12:45 PM Jeff Roberson
wrote:
> On Wed, 15 Jan 2020, Gleb Smirnoff wrote:
>
> > Author: glebius
> > Date: Wed Jan 15 06:05:20 2020
> > New Revision: 356755
> > URL: https://svnweb.freebsd.org/changeset/base/356755
> >
> > Log:
> > Introduce NET_EPOCH_CALL() macro and use it everywhere where we free
> > data based on the network epoch. The macro reverses the argument
> > order of epoch_call(9) - first function, then its argument. NFC
>
> Is there some practical impact of changing the argument order or does it
> just seem more natural to you?
>
Everywhere else in the system, it's fnp, argp for ordering the args...
You'd be hard pressed to find exceptions to this rule. Since this is a new
interface, we should fix it now while we still can :)
Warner
> Jeff
>
> >
> > Modified:
> > head/sys/net/bpf.c
> > head/sys/net/if.c
> > head/sys/net/if_gre.c
> > head/sys/net/if_lagg.c
> > head/sys/net/if_vlan.c
> > head/sys/netinet/in.c
> > head/sys/netinet/in_pcb.c
> > head/sys/netinet/ip_gre.c
> > head/sys/netinet/tcp_ratelimit.c
> > head/sys/netinet6/in6.c
> > head/sys/netinet6/ip6_gre.c
> > head/sys/netpfil/ipfw/nat64/nat64lsn.c
> > head/sys/sys/epoch.h
> >
> > Modified: head/sys/net/bpf.c
> >
> ==
> > --- head/sys/net/bpf.cWed Jan 15 05:48:36 2020(r356754)
> > +++ head/sys/net/bpf.cWed Jan 15 06:05:20 2020(r356755)
> > @@ -274,10 +274,10 @@ static struct filterops bpfread_filtops = {
> > *
> > * 2. An userland application uses ioctl() call to bpf_d descriptor.
> > * All such call are serialized with global lock. BPF filters can be
> > - * changed, but pointer to old filter will be freed using epoch_call().
> > + * changed, but pointer to old filter will be freed using
> NET_EPOCH_CALL().
> > * Thus it should be safe for bpf_tap/bpf_mtap* code to do access to
> > * filter pointers, even if change will happen during bpf_tap execution.
> > - * Destroying of bpf_d descriptor also is doing using epoch_call().
> > + * Destroying of bpf_d descriptor also is doing using NET_EPOCH_CALL().
> > *
> > * 3. An userland application can write packets into bpf_d descriptor.
> > * There we need to be sure, that ifnet won't disappear during
> bpfwrite().
> > @@ -288,7 +288,7 @@ static struct filterops bpfread_filtops = {
> > *
> > * 5. The kernel invokes bpfdetach() on interface destroying. All lists
> > * are modified with global lock held and actual free() is done using
> > - * epoch_call().
> > + * NET_EPOCH_CALL().
> > */
> >
> > static void
> > @@ -314,7 +314,7 @@ bpfif_rele(struct bpf_if *bp)
> >
> > if (!refcount_release(>bif_refcnt))
> > return;
> > - epoch_call(net_epoch_preempt, >epoch_ctx, bpfif_free);
> > + NET_EPOCH_CALL(bpfif_free, >epoch_ctx);
> > }
> >
> > static void
> > @@ -330,7 +330,7 @@ bpfd_rele(struct bpf_d *d)
> >
> > if (!refcount_release(>bd_refcnt))
> > return;
> > - epoch_call(net_epoch_preempt, >epoch_ctx, bpfd_free);
> > + NET_EPOCH_CALL(bpfd_free, >epoch_ctx);
> > }
> >
> > static struct bpf_program_buffer*
> > @@ -2036,8 +2036,7 @@ bpf_setf(struct bpf_d *d, struct bpf_program *fp,
> u_lo
> > BPFD_UNLOCK(d);
> >
> > if (fcode != NULL)
> > - epoch_call(net_epoch_preempt, >epoch_ctx,
> > - bpf_program_buffer_free);
> > + NET_EPOCH_CALL(bpf_program_buffer_free, >epoch_ctx);
> >
> > if (track_event)
> > EVENTHANDLER_INVOKE(bpf_track,
> >
> > Modified: head/sys/net/if.c
> >
> ==
> > --- head/sys/net/if.c Wed Jan 15 05:48:36 2020(r356754)
> > +++ head/sys/net/if.c Wed Jan 15 06:05:20 2020(r356755)
> > @@ -654,7 +654,7 @@ if_free(struct ifnet *ifp)
> > IFNET_WUNLOCK();
> >
> > if (refcount_release(>if_refcount))
> > - epoch_call(net_epoch_preempt, >if_epoch_ctx,
> if_destroy);
> > + NET_EPOCH_CALL(if_destroy, >if_epoch_ctx);
> > CURVNET_RESTORE();
> > }
> >
> > @@ -677,7 +677,7 @@ if_rele(struct ifnet *ifp)
> >
> > if (!refcount_release(>if_refcount))
> > return;
> > - epoch_call(net_epoch_preempt, >if_epoch_ctx, if_destroy);
> > + NET_EPOCH_CALL(if_destroy, >if_epoch_ctx);
> > }
> >
> > void
> > @@ -1826,7 +1826,7 @@ ifa_free(struct ifaddr *ifa)
> > {
> >
> > if (refcount_release(>ifa_refcnt))
> > - epoch_call(net_epoch_preempt, >ifa_epoch_ctx,
> ifa_destroy);
> > + NET_EPOCH_CALL(ifa_destroy, >ifa_epoch_ctx);
> > }
> >
> >
> > @@ -3410,7 +3410,7 @@ if_freemulti(struct ifmultiaddr *ifma)
> > KASSERT(ifma->ifma_refcount == 0, ("if_freemulti_epoch: refcount
> %d",
> > ifma->ifma_refcount));
> >
> > - epoch_call(net_epoch_preempt, >ifma_epoch_ctx,
> if_destroymulti);
> > + NET_EPOCH_CALL(if_destroymulti,
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On Wed, 15 Jan 2020, Gleb Smirnoff wrote:
Author: glebius
Date: Wed Jan 15 06:05:20 2020
New Revision: 356755
URL: https://svnweb.freebsd.org/changeset/base/356755
Log:
Introduce NET_EPOCH_CALL() macro and use it everywhere where we free
data based on the network epoch. The macro reverses the argument
order of epoch_call(9) - first function, then its argument. NFC
Is there some practical impact of changing the argument order or does it
just seem more natural to you?
Jeff
Modified:
head/sys/net/bpf.c
head/sys/net/if.c
head/sys/net/if_gre.c
head/sys/net/if_lagg.c
head/sys/net/if_vlan.c
head/sys/netinet/in.c
head/sys/netinet/in_pcb.c
head/sys/netinet/ip_gre.c
head/sys/netinet/tcp_ratelimit.c
head/sys/netinet6/in6.c
head/sys/netinet6/ip6_gre.c
head/sys/netpfil/ipfw/nat64/nat64lsn.c
head/sys/sys/epoch.h
Modified: head/sys/net/bpf.c
==
--- head/sys/net/bpf.c Wed Jan 15 05:48:36 2020(r356754)
+++ head/sys/net/bpf.c Wed Jan 15 06:05:20 2020(r356755)
@@ -274,10 +274,10 @@ static struct filterops bpfread_filtops = {
*
* 2. An userland application uses ioctl() call to bpf_d descriptor.
* All such call are serialized with global lock. BPF filters can be
- * changed, but pointer to old filter will be freed using epoch_call().
+ * changed, but pointer to old filter will be freed using NET_EPOCH_CALL().
* Thus it should be safe for bpf_tap/bpf_mtap* code to do access to
* filter pointers, even if change will happen during bpf_tap execution.
- * Destroying of bpf_d descriptor also is doing using epoch_call().
+ * Destroying of bpf_d descriptor also is doing using NET_EPOCH_CALL().
*
* 3. An userland application can write packets into bpf_d descriptor.
* There we need to be sure, that ifnet won't disappear during bpfwrite().
@@ -288,7 +288,7 @@ static struct filterops bpfread_filtops = {
*
* 5. The kernel invokes bpfdetach() on interface destroying. All lists
* are modified with global lock held and actual free() is done using
- * epoch_call().
+ * NET_EPOCH_CALL().
*/
static void
@@ -314,7 +314,7 @@ bpfif_rele(struct bpf_if *bp)
if (!refcount_release(>bif_refcnt))
return;
- epoch_call(net_epoch_preempt, >epoch_ctx, bpfif_free);
+ NET_EPOCH_CALL(bpfif_free, >epoch_ctx);
}
static void
@@ -330,7 +330,7 @@ bpfd_rele(struct bpf_d *d)
if (!refcount_release(>bd_refcnt))
return;
- epoch_call(net_epoch_preempt, >epoch_ctx, bpfd_free);
+ NET_EPOCH_CALL(bpfd_free, >epoch_ctx);
}
static struct bpf_program_buffer*
@@ -2036,8 +2036,7 @@ bpf_setf(struct bpf_d *d, struct bpf_program *fp, u_lo
BPFD_UNLOCK(d);
if (fcode != NULL)
- epoch_call(net_epoch_preempt, >epoch_ctx,
- bpf_program_buffer_free);
+ NET_EPOCH_CALL(bpf_program_buffer_free, >epoch_ctx);
if (track_event)
EVENTHANDLER_INVOKE(bpf_track,
Modified: head/sys/net/if.c
==
--- head/sys/net/if.c Wed Jan 15 05:48:36 2020(r356754)
+++ head/sys/net/if.c Wed Jan 15 06:05:20 2020(r356755)
@@ -654,7 +654,7 @@ if_free(struct ifnet *ifp)
IFNET_WUNLOCK();
if (refcount_release(>if_refcount))
- epoch_call(net_epoch_preempt, >if_epoch_ctx, if_destroy);
+ NET_EPOCH_CALL(if_destroy, >if_epoch_ctx);
CURVNET_RESTORE();
}
@@ -677,7 +677,7 @@ if_rele(struct ifnet *ifp)
if (!refcount_release(>if_refcount))
return;
- epoch_call(net_epoch_preempt, >if_epoch_ctx, if_destroy);
+ NET_EPOCH_CALL(if_destroy, >if_epoch_ctx);
}
void
@@ -1826,7 +1826,7 @@ ifa_free(struct ifaddr *ifa)
{
if (refcount_release(>ifa_refcnt))
- epoch_call(net_epoch_preempt, >ifa_epoch_ctx, ifa_destroy);
+ NET_EPOCH_CALL(ifa_destroy, >ifa_epoch_ctx);
}
@@ -3410,7 +3410,7 @@ if_freemulti(struct ifmultiaddr *ifma)
KASSERT(ifma->ifma_refcount == 0, ("if_freemulti_epoch: refcount %d",
ifma->ifma_refcount));
- epoch_call(net_epoch_preempt, >ifma_epoch_ctx, if_destroymulti);
+ NET_EPOCH_CALL(if_destroymulti, >ifma_epoch_ctx);
}
Modified: head/sys/net/if_gre.c
==
--- head/sys/net/if_gre.c Wed Jan 15 05:48:36 2020(r356754)
+++ head/sys/net/if_gre.c Wed Jan 15 06:05:20 2020(r356755)
@@ -392,7 +392,7 @@ gre_delete_tunnel(struct gre_softc *sc)
if ((gs = sc->gre_so) != NULL && CK_LIST_EMPTY(>list)) {
CK_LIST_REMOVE(gs, chain);
soclose(gs->so);
- epoch_call(net_epoch_preempt, >epoch_ctx, gre_sofree);
+ NET_EPOCH_CALL(gre_sofree, >epoch_ctx);
sc->gre_so = NULL;
}
GRE2IFP(sc)->if_drv_flags &=
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On Wed, Jan 15, 2020 at 09:51:02AM +0100, Hans Petter Selasky wrote: H> On 2020-01-15 07:10, Gleb Smirnoff wrote: H> > I really want to reverse the argument order of epoch_call() as well. H> > The current order is really backwards: H> > H> > void H> > epoch_call(epoch_t epoch, epoch_context_t ctx, H> > void (*callback)(epoch_context_t)); H> > H> > Suggested declaration is: H> > H> > void H> > epoch_call(epoch_t epoch, epoch_context_t ctx, H> > void (*callback)(epoch_context_t)); H> H> I think he meant to put the ctx argument last. Look at how the function H> is implemented to see if that makes any sense, I.E. how arguments are H> optimised. Yes, of course. I had too little tea last night and didn't swap arguments after copy-n-paste. Suggested prototype is: void epoch_call(epoch_t epoch, void (*callback)(epoch_context_t), epoch_context_t ctx); H> Is this *want* just because of "function, argument" is better than H> "argument, function" ? Sure. There is no practical impact on how a CPU will execute. It is all about how a human reads a code. -- Gleb Smirnoff ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On 2020-01-15 07:10, Gleb Smirnoff wrote: I really want to reverse the argument order of epoch_call() as well. The current order is really backwards: void epoch_call(epoch_t epoch, epoch_context_t ctx, void (*callback)(epoch_context_t)); Suggested declaration is: void epoch_call(epoch_t epoch, epoch_context_t ctx, void (*callback)(epoch_context_t)); Hi, I think he meant to put the ctx argument last. Look at how the function is implemented to see if that makes any sense, I.E. how arguments are optimised. > epoch_call(epoch_t epoch, epoch_context_t ctx, > void (*callback)(epoch_context_t)); Is this *want* just because of "function, argument" is better than "argument, function" ? --HPS ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
On Tue, Jan 14, 2020 at 10:10:46PM -0800, Gleb Smirnoff wrote: > void > epoch_call(epoch_t epoch, epoch_context_t ctx, > void (*callback)(epoch_context_t)); > > void > epoch_call(epoch_t epoch, epoch_context_t ctx, > void (*callback)(epoch_context_t)); I'm staring at these *really* hard and ... can't tell the difference? mcl ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
Aren’t the current and suggested the same there or do I need more coffee this morning? On Wed, 15 Jan 2020 at 06:10, Gleb Smirnoff wrote: > Hi, > > On Wed, Jan 15, 2020 at 06:05:20AM +, Gleb Smirnoff wrote: > T> Log: > T> Introduce NET_EPOCH_CALL() macro and use it everywhere where we free > T> data based on the network epoch. The macro reverses the argument > T> order of epoch_call(9) - first function, then its argument. NFC > > I really want to reverse the argument order of epoch_call() as well. > The current order is really backwards: > > void > epoch_call(epoch_t epoch, epoch_context_t ctx, > void (*callback)(epoch_context_t)); > > Suggested declaration is: > > void > epoch_call(epoch_t epoch, epoch_context_t ctx, > void (*callback)(epoch_context_t)); > > This will be a very easy change, since today function is > used just in few places. > > Before branching stable/12 we intentionally put this > note in epoch.9 manual page: > > NOTES > The epoch kernel programming interface is under development and is > subject to change. > > Any objections? > > -- > Gleb Smirnoff > ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r356755 - in head/sys: net netinet netinet6 netpfil/ipfw/nat64 sys
Hi, On Wed, Jan 15, 2020 at 06:05:20AM +, Gleb Smirnoff wrote: T> Log: T> Introduce NET_EPOCH_CALL() macro and use it everywhere where we free T> data based on the network epoch. The macro reverses the argument T> order of epoch_call(9) - first function, then its argument. NFC I really want to reverse the argument order of epoch_call() as well. The current order is really backwards: void epoch_call(epoch_t epoch, epoch_context_t ctx, void (*callback)(epoch_context_t)); Suggested declaration is: void epoch_call(epoch_t epoch, epoch_context_t ctx, void (*callback)(epoch_context_t)); This will be a very easy change, since today function is used just in few places. Before branching stable/12 we intentionally put this note in epoch.9 manual page: NOTES The epoch kernel programming interface is under development and is subject to change. Any objections? -- Gleb Smirnoff ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
