Hello, > On Wed, Oct 28, 2015 at 06:19:48PM +0100, Alexandr Nedvedicky wrote: > > The idea has been proposed by Claudio at Varazdin. > > I guess the idea is to eliminate the workq. Or is ther naother > reason to change it?
the primary goal is to kill work queues. > > Comments inline > thank you very much for very good code review. > > - *nadd = io.pfrio_nadd; > > - return (0); > > + io.pfrio_size = 1; /* TODO: check .pfrio_size is needed */ > > + for (i = 0; (i < size) && (rv == 0); i++) { > > rv is unitialized in the first interation it's fixed by change below: @@ -184,7 +184,8 @@ int *nadd, int flags) { struct pfioc_table io; - int i, rv, add = 0; + int i, add = 0; + int rv = 0; > > > + io.pfrio_buffer = addr++; > > + rv = ioctl(dev, DIOCRADDADDR, &io); > > I would suggest to return (-1) if ioctl fails... I'll write my response in email answering your note on 'illusion of atomicity' > > pfr_add_addr() handles exactly 1 address, don't pass io->pfrio_size. > yes that's true... error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer, - io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL); + io->pfrio_flags | PFR_FLAG_USERIOCTL); break; > > > > int > > +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int > > flags) > > Do not pass size. -pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int flags) +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int flags) > > Should you check for !(flags & PFR_FLAG_DUMMY) here? It was done > in the old code before pfr_insert_kentries(). > > > + rv = pfr_insert_kentry(kt, &ad, tzero); > > The old code ignored wether pfr_insert_kentries() succeeded. > thanks for catching this. @@ -288,20 +288,20 @@ senderr(EINVAL); p = pfr_lookup_addr(kt, &ad, 1); if (p == NULL) { - rv = pfr_insert_kentry(kt, &ad, tzero); + if (!(flags & PFR_FLAG_DUMMY)) + rv = pfr_insert_kentry(kt, &ad, tzero); + else + rv = 0; } > > No { } for one line if block. there is no longer one line if block after fixing PFR_FLAG_DUMMY flag. > > PFR_FB_DUPLICATE was used when there were two identical addresses > in the passed list. This cannot happen anymore. It should be > PFR_FB_NONE in this case. > > > + } else if (rv != 0) { > > + ad.pfra_fback = PFR_FB_NONE; > > + } else { > > + ad.pfra_fback = PFR_FB_ADDED; > > + } > > Perhaps write this block as > > if (p == NULL) > ad.pfra_fback = PFR_FB_ADDED; > else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not)) > ad.pfra_fback = PFR_FB_CONFLICT; > else > ad.pfra_fback = PFR_FB_NONE; > I thinks we still must check rv coming from pfr_insert_kentry(): if (flags & PFR_FLAG_FEEDBACK) { - if (p != NULL) { - if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not) - ad.pfra_fback = PFR_FB_CONFLICT; - else - ad.pfra_fback = PFR_FB_DUPLICATE; - } else if (rv != 0) { + if (p == NULL) + ad.pfra_fback = (rv == 0) ? PFR_FB_ADDED : PFR_FB_NONE; + else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not) + ad.pfra_fback = PFR_FB_CONFLICT; + else ad.pfra_fback = PFR_FB_NONE; - } else { - ad.pfra_fback = PFR_FB_ADDED; - } > > +_bad: > > + if (flags & PFR_FLAG_FEEDBACK) > > + pfr_reset_feedback(addr, size, flags); > > Don't use size, it must be 1. the size argument got dropped. > > > + return (rv); > > rv may be unitialized thanks for catching that. > > pfr_add_addrs() is not used anymore, remove it. pfr_add_addrs() is gone in new patch. thanks and regards sasha --------8<---------------8<-----------------8<-------- Index: sbin/pfctl/pfctl_radix.c =================================================================== RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v retrieving revision 1.32 diff -u -p -r1.32 pfctl_radix.c --- sbin/pfctl/pfctl_radix.c 21 Jan 2015 21:50:33 -0000 1.32 +++ sbin/pfctl/pfctl_radix.c 9 Nov 2015 20:43:52 -0000 @@ -184,6 +184,8 @@ pfr_add_addrs(struct pfr_table *tbl, str int *nadd, int flags) { struct pfioc_table io; + int i, add = 0; + int rv = 0; if (tbl == NULL || size < 0 || (size && addr == NULL)) { errno = EINVAL; @@ -192,14 +194,19 @@ pfr_add_addrs(struct pfr_table *tbl, str bzero(&io, sizeof io); io.pfrio_flags = flags; io.pfrio_table = *tbl; - io.pfrio_buffer = addr; io.pfrio_esize = sizeof(*addr); - io.pfrio_size = size; - if (ioctl(dev, DIOCRADDADDRS, &io)) - return (-1); - if (nadd != NULL) - *nadd = io.pfrio_nadd; - return (0); + io.pfrio_size = 1; + for (i = 0; (i < size) && (rv == 0); i++) { + io.pfrio_buffer = addr++; + if (ioctl(dev, DIOCRADDADDR, &io) == -1) + rv = -1; + add++; + } + + if ((rv == 0) && (nadd != NULL)) + *nadd = add; + + return (rv); } int Index: sys/net/pf_ioctl.c =================================================================== RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.291 diff -u -p -r1.291 pf_ioctl.c --- sys/net/pf_ioctl.c 13 Oct 2015 19:32:31 -0000 1.291 +++ sys/net/pf_ioctl.c 9 Nov 2015 20:44:21 -0000 @@ -834,7 +834,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a case DIOCRGETTSTATS: case DIOCRCLRTSTATS: case DIOCRCLRADDRS: - case DIOCRADDADDRS: + case DIOCRADDADDR: case DIOCRDELADDRS: case DIOCRSETADDRS: case DIOCRGETASTATS: @@ -887,7 +887,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a case DIOCRDELTABLES: case DIOCRCLRTSTATS: case DIOCRCLRADDRS: - case DIOCRADDADDRS: + case DIOCRADDADDR: case DIOCRDELADDRS: case DIOCRSETADDRS: case DIOCRSETTFLAGS: @@ -1816,16 +1816,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a break; } - case DIOCRADDADDRS: { + case DIOCRADDADDR: { struct pfioc_table *io = (struct pfioc_table *)addr; if (io->pfrio_esize != sizeof(struct pfr_addr)) { error = ENODEV; break; } - error = pfr_add_addrs(&io->pfrio_table, io->pfrio_buffer, - io->pfrio_size, &io->pfrio_nadd, io->pfrio_flags | - PFR_FLAG_USERIOCTL); + error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer, + io->pfrio_flags | PFR_FLAG_USERIOCTL); break; } Index: sys/net/pf_table.c =================================================================== RCS file: /cvs/src/sys/net/pf_table.c,v retrieving revision 1.116 diff -u -p -r1.116 pf_table.c --- sys/net/pf_table.c 3 Nov 2015 22:10:33 -0000 1.116 +++ sys/net/pf_table.c 9 Nov 2015 20:44:23 -0000 @@ -266,14 +266,12 @@ pfr_clr_addrs(struct pfr_table *tbl, int } int -pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size, - int *nadd, int flags) +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int flags) { - struct pfr_ktable *kt, *tmpkt; - struct pfr_kentryworkq workq; - struct pfr_kentry *p, *q; + struct pfr_ktable *kt; + struct pfr_kentry *p; struct pfr_addr ad; - int i, rv, xadd = 0; + int rv = 0; time_t tzero = time_second; ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK); @@ -284,61 +282,34 @@ pfr_add_addrs(struct pfr_table *tbl, str return (ESRCH); if (kt->pfrkt_flags & PFR_TFLAG_CONST) return (EPERM); - tmpkt = pfr_create_ktable(&pfr_nulltable, 0, 0, - !(flags & PFR_FLAG_USERIOCTL)); - if (tmpkt == NULL) - return (ENOMEM); - SLIST_INIT(&workq); - for (i = 0; i < size; i++) { - YIELD(i, flags & PFR_FLAG_USERIOCTL); - if (COPYIN(addr+i, &ad, sizeof(ad), flags)) + if (COPYIN(addr, &ad, sizeof(ad), flags)) + senderr(EFAULT); + if (pfr_validate_addr(&ad)) + senderr(EINVAL); + p = pfr_lookup_addr(kt, &ad, 1); + if (p == NULL) { + if (!(flags & PFR_FLAG_DUMMY)) + rv = pfr_insert_kentry(kt, &ad, tzero); + else + rv = 0; + } + + if (flags & PFR_FLAG_FEEDBACK) { + if (p == NULL) + ad.pfra_fback = (rv == 0) ? PFR_FB_ADDED : PFR_FB_NONE; + else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not) + ad.pfra_fback = PFR_FB_CONFLICT; + else + ad.pfra_fback = PFR_FB_NONE; + + if (COPYOUT(&ad, addr, sizeof(ad), flags)) senderr(EFAULT); - if (pfr_validate_addr(&ad)) - senderr(EINVAL); - p = pfr_lookup_addr(kt, &ad, 1); - q = pfr_lookup_addr(tmpkt, &ad, 1); - if (flags & PFR_FLAG_FEEDBACK) { - if (q != NULL) - ad.pfra_fback = PFR_FB_DUPLICATE; - else if (p == NULL) - ad.pfra_fback = PFR_FB_ADDED; - else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != - ad.pfra_not) - ad.pfra_fback = PFR_FB_CONFLICT; - else - ad.pfra_fback = PFR_FB_NONE; - } - if (p == NULL && q == NULL) { - p = pfr_create_kentry(&ad); - if (p == NULL) - senderr(ENOMEM); - if (pfr_route_kentry(tmpkt, p)) { - pfr_destroy_kentry(p); - ad.pfra_fback = PFR_FB_NONE; - } else { - SLIST_INSERT_HEAD(&workq, p, pfrke_workq); - xadd++; - } - } - if (flags & PFR_FLAG_FEEDBACK) - if (COPYOUT(&ad, addr+i, sizeof(ad), flags)) - senderr(EFAULT); } - pfr_clean_node_mask(tmpkt, &workq); - if (!(flags & PFR_FLAG_DUMMY)) { - pfr_insert_kentries(kt, &workq, tzero); - } else - pfr_destroy_kentries(&workq); - if (nadd != NULL) - *nadd = xadd; - pfr_destroy_ktable(tmpkt, 0); + return (0); _bad: - pfr_clean_node_mask(tmpkt, &workq); - pfr_destroy_kentries(&workq); if (flags & PFR_FLAG_FEEDBACK) - pfr_reset_feedback(addr, size, flags); - pfr_destroy_ktable(tmpkt, 0); + pfr_reset_feedback(addr, 1, flags); return (rv); } Index: sys/net/pfvar.h =================================================================== RCS file: /cvs/src/sys/net/pfvar.h,v retrieving revision 1.422 diff -u -p -r1.422 pfvar.h --- sys/net/pfvar.h 30 Oct 2015 11:33:55 -0000 1.422 +++ sys/net/pfvar.h 9 Nov 2015 20:44:24 -0000 @@ -1613,7 +1613,7 @@ struct pfioc_iface { #define DIOCRGETTSTATS _IOWR('D', 64, struct pfioc_table) #define DIOCRCLRTSTATS _IOWR('D', 65, struct pfioc_table) #define DIOCRCLRADDRS _IOWR('D', 66, struct pfioc_table) -#define DIOCRADDADDRS _IOWR('D', 67, struct pfioc_table) +#define DIOCRADDADDR _IOWR('D', 67, struct pfioc_table) #define DIOCRDELADDRS _IOWR('D', 68, struct pfioc_table) #define DIOCRSETADDRS _IOWR('D', 69, struct pfioc_table) #define DIOCRGETADDRS _IOWR('D', 70, struct pfioc_table) @@ -1789,8 +1789,7 @@ int pfr_clr_tstats(struct pfr_table *, i int pfr_set_tflags(struct pfr_table *, int, int, int, int *, int *, int); int pfr_clr_addrs(struct pfr_table *, int *, int); int pfr_insert_kentry(struct pfr_ktable *, struct pfr_addr *, time_t); -int pfr_add_addrs(struct pfr_table *, struct pfr_addr *, int, int *, - int); +int pfr_add_addr(struct pfr_table *, struct pfr_addr *, int); int pfr_del_addrs(struct pfr_table *, struct pfr_addr *, int, int *, int); int pfr_set_addrs(struct pfr_table *, struct pfr_addr *, int, int *, Index: sbin/pfctl/pfctl_radix.c =================================================================== RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v retrieving revision 1.32 diff -u -p -r1.32 pfctl_radix.c --- sbin/pfctl/pfctl_radix.c 21 Jan 2015 21:50:33 -0000 1.32 +++ sbin/pfctl/pfctl_radix.c 9 Nov 2015 20:46:39 -0000 @@ -184,6 +184,8 @@ pfr_add_addrs(struct pfr_table *tbl, str int *nadd, int flags) { struct pfioc_table io; + int i, add = 0; + int rv = 0; if (tbl == NULL || size < 0 || (size && addr == NULL)) { errno = EINVAL; @@ -192,14 +194,19 @@ pfr_add_addrs(struct pfr_table *tbl, str bzero(&io, sizeof io); io.pfrio_flags = flags; io.pfrio_table = *tbl; - io.pfrio_buffer = addr; io.pfrio_esize = sizeof(*addr); - io.pfrio_size = size; - if (ioctl(dev, DIOCRADDADDRS, &io)) - return (-1); - if (nadd != NULL) - *nadd = io.pfrio_nadd; - return (0); + io.pfrio_size = 1; + for (i = 0; (i < size) && (rv == 0); i++) { + io.pfrio_buffer = addr++; + if (ioctl(dev, DIOCRADDADDR, &io) == -1) + rv = -1; + add++; + } + + if ((rv == 0) && (nadd != NULL)) + *nadd = add; + + return (rv); } int Index: sys/net/pf_ioctl.c =================================================================== RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.291 diff -u -p -r1.291 pf_ioctl.c --- sys/net/pf_ioctl.c 13 Oct 2015 19:32:31 -0000 1.291 +++ sys/net/pf_ioctl.c 9 Nov 2015 20:47:09 -0000 @@ -834,7 +834,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a case DIOCRGETTSTATS: case DIOCRCLRTSTATS: case DIOCRCLRADDRS: - case DIOCRADDADDRS: + case DIOCRADDADDR: case DIOCRDELADDRS: case DIOCRSETADDRS: case DIOCRGETASTATS: @@ -887,7 +887,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a case DIOCRDELTABLES: case DIOCRCLRTSTATS: case DIOCRCLRADDRS: - case DIOCRADDADDRS: + case DIOCRADDADDR: case DIOCRDELADDRS: case DIOCRSETADDRS: case DIOCRSETTFLAGS: @@ -1816,16 +1816,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a break; } - case DIOCRADDADDRS: { + case DIOCRADDADDR: { struct pfioc_table *io = (struct pfioc_table *)addr; if (io->pfrio_esize != sizeof(struct pfr_addr)) { error = ENODEV; break; } - error = pfr_add_addrs(&io->pfrio_table, io->pfrio_buffer, - io->pfrio_size, &io->pfrio_nadd, io->pfrio_flags | - PFR_FLAG_USERIOCTL); + error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer, + io->pfrio_flags | PFR_FLAG_USERIOCTL); break; } Index: sys/net/pf_table.c =================================================================== RCS file: /cvs/src/sys/net/pf_table.c,v retrieving revision 1.116 diff -u -p -r1.116 pf_table.c --- sys/net/pf_table.c 3 Nov 2015 22:10:33 -0000 1.116 +++ sys/net/pf_table.c 9 Nov 2015 20:47:10 -0000 @@ -266,14 +266,12 @@ pfr_clr_addrs(struct pfr_table *tbl, int } int -pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size, - int *nadd, int flags) +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int flags) { - struct pfr_ktable *kt, *tmpkt; - struct pfr_kentryworkq workq; - struct pfr_kentry *p, *q; + struct pfr_ktable *kt; + struct pfr_kentry *p; struct pfr_addr ad; - int i, rv, xadd = 0; + int rv = 0; time_t tzero = time_second; ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK); @@ -284,61 +282,34 @@ pfr_add_addrs(struct pfr_table *tbl, str return (ESRCH); if (kt->pfrkt_flags & PFR_TFLAG_CONST) return (EPERM); - tmpkt = pfr_create_ktable(&pfr_nulltable, 0, 0, - !(flags & PFR_FLAG_USERIOCTL)); - if (tmpkt == NULL) - return (ENOMEM); - SLIST_INIT(&workq); - for (i = 0; i < size; i++) { - YIELD(i, flags & PFR_FLAG_USERIOCTL); - if (COPYIN(addr+i, &ad, sizeof(ad), flags)) + if (COPYIN(addr, &ad, sizeof(ad), flags)) + senderr(EFAULT); + if (pfr_validate_addr(&ad)) + senderr(EINVAL); + p = pfr_lookup_addr(kt, &ad, 1); + if (p == NULL) { + if (!(flags & PFR_FLAG_DUMMY)) + rv = pfr_insert_kentry(kt, &ad, tzero); + else + rv = 0; + } + + if (flags & PFR_FLAG_FEEDBACK) { + if (p == NULL) + ad.pfra_fback = (rv == 0) ? PFR_FB_ADDED : PFR_FB_NONE; + else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not) + ad.pfra_fback = PFR_FB_CONFLICT; + else + ad.pfra_fback = PFR_FB_NONE; + + if (COPYOUT(&ad, addr, sizeof(ad), flags)) senderr(EFAULT); - if (pfr_validate_addr(&ad)) - senderr(EINVAL); - p = pfr_lookup_addr(kt, &ad, 1); - q = pfr_lookup_addr(tmpkt, &ad, 1); - if (flags & PFR_FLAG_FEEDBACK) { - if (q != NULL) - ad.pfra_fback = PFR_FB_DUPLICATE; - else if (p == NULL) - ad.pfra_fback = PFR_FB_ADDED; - else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != - ad.pfra_not) - ad.pfra_fback = PFR_FB_CONFLICT; - else - ad.pfra_fback = PFR_FB_NONE; - } - if (p == NULL && q == NULL) { - p = pfr_create_kentry(&ad); - if (p == NULL) - senderr(ENOMEM); - if (pfr_route_kentry(tmpkt, p)) { - pfr_destroy_kentry(p); - ad.pfra_fback = PFR_FB_NONE; - } else { - SLIST_INSERT_HEAD(&workq, p, pfrke_workq); - xadd++; - } - } - if (flags & PFR_FLAG_FEEDBACK) - if (COPYOUT(&ad, addr+i, sizeof(ad), flags)) - senderr(EFAULT); } - pfr_clean_node_mask(tmpkt, &workq); - if (!(flags & PFR_FLAG_DUMMY)) { - pfr_insert_kentries(kt, &workq, tzero); - } else - pfr_destroy_kentries(&workq); - if (nadd != NULL) - *nadd = xadd; - pfr_destroy_ktable(tmpkt, 0); + return (0); _bad: - pfr_clean_node_mask(tmpkt, &workq); - pfr_destroy_kentries(&workq); if (flags & PFR_FLAG_FEEDBACK) - pfr_reset_feedback(addr, size, flags); - pfr_destroy_ktable(tmpkt, 0); + pfr_reset_feedback(addr, 1, flags); return (rv); } Index: sys/net/pfvar.h =================================================================== RCS file: /cvs/src/sys/net/pfvar.h,v retrieving revision 1.422 diff -u -p -r1.422 pfvar.h --- sys/net/pfvar.h 30 Oct 2015 11:33:55 -0000 1.422 +++ sys/net/pfvar.h 9 Nov 2015 20:47:12 -0000 @@ -1613,7 +1613,7 @@ struct pfioc_iface { #define DIOCRGETTSTATS _IOWR('D', 64, struct pfioc_table) #define DIOCRCLRTSTATS _IOWR('D', 65, struct pfioc_table) #define DIOCRCLRADDRS _IOWR('D', 66, struct pfioc_table) -#define DIOCRADDADDRS _IOWR('D', 67, struct pfioc_table) +#define DIOCRADDADDR _IOWR('D', 67, struct pfioc_table) #define DIOCRDELADDRS _IOWR('D', 68, struct pfioc_table) #define DIOCRSETADDRS _IOWR('D', 69, struct pfioc_table) #define DIOCRGETADDRS _IOWR('D', 70, struct pfioc_table) @@ -1789,8 +1789,7 @@ int pfr_clr_tstats(struct pfr_table *, i int pfr_set_tflags(struct pfr_table *, int, int, int, int *, int *, int); int pfr_clr_addrs(struct pfr_table *, int *, int); int pfr_insert_kentry(struct pfr_ktable *, struct pfr_addr *, time_t); -int pfr_add_addrs(struct pfr_table *, struct pfr_addr *, int, int *, - int); +int pfr_add_addr(struct pfr_table *, struct pfr_addr *, int); int pfr_del_addrs(struct pfr_table *, struct pfr_addr *, int, int *, int); int pfr_set_addrs(struct pfr_table *, struct pfr_addr *, int, int *,