[Bug 230619] pf: tables use non SMP-friendly counters
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=230619 Mark Linimon changed: What|Removed |Added Keywords||patch Assignee|b...@freebsd.org|p...@freebsd.org -- You are receiving this mail because: You are the assignee for the bug. ___ freebsd-pf@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-pf To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"
Re: pf tables locking
On Tuesday, 14 August 2018 15:44:52 CEST Ermal Luçi wrote: > If you really want to spend time on it, the best option is to pull out the > pool concept used by the rules/nat... and manage it outside of the > rules/states but in its own module referenced by the former ones. Do you mean as separate kernel module? Or totally outside of kernel? I was considering doing this outside of kernel by providing a weighted round-robin algorithm but that would still require most of the patches as for doing it within kernel, in order to get counters working for redirection tables and state counter per table element, which both are missing in kernel now. > This would allow extensibility and propper reasoning about it. It might be the late hour but I really don't see how it would be extensible. Please be more specific. -- | pozdrawiam / greetings | powered by Debian, FreeBSD and CentOS | | Kajetan Staszkiewicz | jabber,email: vegeta()tuxpowered net | |Vegeta | www: http://vegeta.tuxpowered.net | `^---' signature.asc Description: This is a digitally signed message part.
Re: pf tables locking
(sorry for the top post) If you really want to spend time on it, the best option is to pull out the pool concept used by the rules/nat... and manage it outside of the rules/states but in its own module referenced by the former ones. This would allow extensibility and propper reasoning about it. On Tue, Aug 14, 2018 at 9:35 AM, Kajetan Staszkiewicz wrote: > On Tuesday, 14 August 2018 16:15:48 CEST Kristof Provost wrote: > > On 14 Aug 2018, at 0:32, Kajetan Staszkiewicz wrote: > > > On Monday, 13 August 2018 17:59:15 CEST Kristof Provost wrote: > > > How about this? > > > > > > https://github.com/innogames/freebsd/commit/ > > > d44a0d9487285fac8ed1d7372cc99cca83f616e6 > > > > That looks good to me. > > There’s a few minor issues, things like inconsistent indentation and > > overly long lines, but that’s about the only criticism I have. > > I fixed some issues with unallocated counters and submitted bug 230619. > > > I see. I’m not quite sure yet if that’s a feature we want to import > > or not, > > but at least your ‘support’ patches should probably go in. The above > > one certainly. > > There are some more things which require changes before I can do least- > connections balancing. > > If you have a moment, please have a look at https://github.com/innogames/ > freebsd/commits/iglb/11.2/GetOnWithIt_2 , maybe some of those things can > get > imported anyway, like full support for counters of states. > > > >> Yeah, that bug is still on my todo list somewhere, but things are > > >> extremely > > >> hectic at the moment, and I can’t make any promises about when > > >> I’ll have > > >> time for it. > > > > > > I thought that was rather on my todo :) > > > > I’m not going to stop you. I love it when other people do the work ;) > > Since I have you here, let me explain the issues I see with pf_map_addr(). > For > round-robin target a list of interface,table pairs can be specified. This > list > is iterated and within each table addresses are iterated too. There is no > locking around it "because performance is assumed more important than > round- > robin precision" according to comment in code. > > Yet I believe there are way more serious issues possible with the current > approach. Interface is in fact picked up outside of pf_map_addr(). Another > thread could have already moved the rpool->counter to another table for > which > the interface is not valid anymore. > > I came up with this: https://github.com/innogames/freebsd/commit/ > 61ffb96a4dc948a0b06204ff39210c0578f77f08 although without locking this is > still not really a solution. It only moves interface selection to inside > of > pf_map_addr() > > Another one is https://github.com/innogames/freebsd/commit/ > 8fe6cd2d820052d2166afbaa311f34318a41db48 which stores table used for > loadbalancing in state and src_node. Then the table can be used for state > counting. > > The 2 patches above are also included in the first link I gave above. > > -- > | pozdrawiam / greetings | powered by Debian, FreeBSD and CentOS | > | Kajetan Staszkiewicz | jabber,email: vegeta()tuxpowered net | > |Vegeta | www: http://vegeta.tuxpowered.net | > `^---' > > -- > Ermal > ___ freebsd-pf@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-pf To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"
Re: pf tables locking
On Tuesday, 14 August 2018 16:15:48 CEST Kristof Provost wrote: > On 14 Aug 2018, at 0:32, Kajetan Staszkiewicz wrote: > > On Monday, 13 August 2018 17:59:15 CEST Kristof Provost wrote: > > How about this? > > > > https://github.com/innogames/freebsd/commit/ > > d44a0d9487285fac8ed1d7372cc99cca83f616e6 > > That looks good to me. > There’s a few minor issues, things like inconsistent indentation and > overly long lines, but that’s about the only criticism I have. I fixed some issues with unallocated counters and submitted bug 230619. > I see. I’m not quite sure yet if that’s a feature we want to import > or not, > but at least your ‘support’ patches should probably go in. The above > one certainly. There are some more things which require changes before I can do least- connections balancing. If you have a moment, please have a look at https://github.com/innogames/ freebsd/commits/iglb/11.2/GetOnWithIt_2 , maybe some of those things can get imported anyway, like full support for counters of states. > >> Yeah, that bug is still on my todo list somewhere, but things are > >> extremely > >> hectic at the moment, and I can’t make any promises about when > >> I’ll have > >> time for it. > > > > I thought that was rather on my todo :) > > I’m not going to stop you. I love it when other people do the work ;) Since I have you here, let me explain the issues I see with pf_map_addr(). For round-robin target a list of interface,table pairs can be specified. This list is iterated and within each table addresses are iterated too. There is no locking around it "because performance is assumed more important than round- robin precision" according to comment in code. Yet I believe there are way more serious issues possible with the current approach. Interface is in fact picked up outside of pf_map_addr(). Another thread could have already moved the rpool->counter to another table for which the interface is not valid anymore. I came up with this: https://github.com/innogames/freebsd/commit/ 61ffb96a4dc948a0b06204ff39210c0578f77f08 although without locking this is still not really a solution. It only moves interface selection to inside of pf_map_addr() Another one is https://github.com/innogames/freebsd/commit/ 8fe6cd2d820052d2166afbaa311f34318a41db48 which stores table used for loadbalancing in state and src_node. Then the table can be used for state counting. The 2 patches above are also included in the first link I gave above. -- | pozdrawiam / greetings | powered by Debian, FreeBSD and CentOS | | Kajetan Staszkiewicz | jabber,email: vegeta()tuxpowered net | |Vegeta | www: http://vegeta.tuxpowered.net | `^---' signature.asc Description: This is a digitally signed message part.
Re: pf tables locking
On 14 Aug 2018, at 0:32, Kajetan Staszkiewicz wrote: On Monday, 13 August 2018 17:59:15 CEST Kristof Provost wrote: How about this? https://github.com/innogames/freebsd/commit/ d44a0d9487285fac8ed1d7372cc99cca83f616e6 That looks good to me. There’s a few minor issues, things like inconsistent indentation and overly long lines, but that’s about the only criticism I have. Do you have a bit more information about your use case? What are you trying to accomplish with this change? I have a loadbalancer which uses pf and route-to targets. After a server is added to a pool, I want this server to immediately take over much traffic. With round-robin the server receives new clients rather slowly. If kernel could measure amount of states per table entry, I could send new clients to this new server until it serves as many clients as other servers. I see. I’m not quite sure yet if that’s a feature we want to import or not, but at least your ‘support’ patches should probably go in. The above one certainly. There are some more issues I found around pf_map_addr. Some of them I mentioned in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229092. Some more came out while working on this least-states loadbalancing. I will group them into something meaningful and make another PR for them. Yeah, that bug is still on my todo list somewhere, but things are extremely hectic at the moment, and I can’t make any promises about when I’ll have time for it. I thought that was rather on my todo :) I’m not going to stop you. I love it when other people do the work ;) Regards, Kristof ___ freebsd-pf@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-pf To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"
