Re: svn commit: r276747 - head/sys/netpfil/pf
Cool. Having the PF + VIMAGE issues resolved and committed to HEAD is what I am most interested in. When you have a patch that you are happy with, would you be OK with getting it into Phabricator? It would be good to have it in there, so that multiple people can review it, including Gleb. Unfortunately, currently only committers can open reviews in Phabricator, so we would need to get your patch to a committer to enter in Phabricator. I can help with that, or you can hand off to another committer if you are more comfortable with that. Thanks! -- Craig On Mon, Feb 16, 2015 at 12:58 AM, Nikos Vassiliadis nv...@gmx.com wrote: Hi Gleb Craig, I am already working on the issues that need to be fixed, namely: 1) all the changes from projects/pf 2) unloading the module 3) one purge thread per pf instance will remain until we have more experience It's almost ready, I am trying to fix unloading the module. As I am not at home this week I will send you a patch for review on Monday. Best regards, Nikos On 02/16/15 09:02, Craig Rodrigues wrote: On Sun, Feb 15, 2015 at 11:46 PM, Gleb Smirnoff gleb...@freebsd.org wrote: On Sun, Feb 15, 2015 at 11:36:17PM -0800, Craig Rodrigues wrote: C On Sun, Feb 15, 2015 at 5:25 PM, Gleb Smirnoff gleb...@freebsd.org wrote: C C On Sun, Feb 15, 2015 at 01:33:19PM -0800, Craig Rodrigues wrote: C C By the way, it would be helpful if you could provide feedback in C C Phabricator. When I created those Phabricator reviews, I added C C you as a reviewer to all of them, so you can't say that you didn't see C C the patches. C C You did not provide feedback on any of them: C C C C https://reviews.freebsd.org/D1309 C C https://reviews.freebsd.org/D1312 C C https://reviews.freebsd.org/D1313 C C https://reviews.freebsd.org/D1315 C C C C Please take some time to go and provide feedback in those C C reviews, so that a better patch can be made that makes you happy. C C Did you address all problems that arised after code was committed? C C Please do, otherwise my review would require me to cut-n-paste from C my own emails. C C C Yes, please cut and paste from your e-mails, and put in the reviews. No, I will not do this. You know right now that you have problems in the aforementioned phab URLs, and you are asking me to look at patches at to point them out to you. This is your task, not mine. Please address all already known problems and update the phab revisions. C It's easier to follow in the individual reviews because there are different C changes C in each review, rather than one big revert, which is what you did. I'm already starting to repeat myself. I did a big revert, because the first change wasn't compilable, later changes fixed compilation failures, but introduced unacceptable bugs. That's why I was forced to back out all chain. It's not clear to me what parts you find unacceptable, and what parts you find acceptable. I added you to the reviews in Phabricator from the beginning, so it would have been nice if you could have provided the review feedback there. Since you are slow to provide feedback when I asked you to be a reviewer in Phabricator, but you are quick to revert changes, and provide unclear feedback during your reversion, this makes it very hard to make forward progress in this area. In the past, multiple like Nikos and Martin Matuska have been providing patches in your projects/pf branch, but you don't seem to have the time/interest to push these patches back into head. I've seen that Martin Matuska stopped waiting for you to merge the pf branch to head, and started cherry -- Craig ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
Hi Gleb Craig, I am already working on the issues that need to be fixed, namely: 1) all the changes from projects/pf 2) unloading the module 3) one purge thread per pf instance will remain until we have more experience It's almost ready, I am trying to fix unloading the module. As I am not at home this week I will send you a patch for review on Monday. Best regards, Nikos On 02/16/15 09:02, Craig Rodrigues wrote: On Sun, Feb 15, 2015 at 11:46 PM, Gleb Smirnoff gleb...@freebsd.org wrote: On Sun, Feb 15, 2015 at 11:36:17PM -0800, Craig Rodrigues wrote: C On Sun, Feb 15, 2015 at 5:25 PM, Gleb Smirnoff gleb...@freebsd.org wrote: C C On Sun, Feb 15, 2015 at 01:33:19PM -0800, Craig Rodrigues wrote: C C By the way, it would be helpful if you could provide feedback in C C Phabricator. When I created those Phabricator reviews, I added C C you as a reviewer to all of them, so you can't say that you didn't see C C the patches. C C You did not provide feedback on any of them: C C C C https://reviews.freebsd.org/D1309 C C https://reviews.freebsd.org/D1312 C C https://reviews.freebsd.org/D1313 C C https://reviews.freebsd.org/D1315 C C C C Please take some time to go and provide feedback in those C C reviews, so that a better patch can be made that makes you happy. C C Did you address all problems that arised after code was committed? C C Please do, otherwise my review would require me to cut-n-paste from C my own emails. C C C Yes, please cut and paste from your e-mails, and put in the reviews. No, I will not do this. You know right now that you have problems in the aforementioned phab URLs, and you are asking me to look at patches at to point them out to you. This is your task, not mine. Please address all already known problems and update the phab revisions. C It's easier to follow in the individual reviews because there are different C changes C in each review, rather than one big revert, which is what you did. I'm already starting to repeat myself. I did a big revert, because the first change wasn't compilable, later changes fixed compilation failures, but introduced unacceptable bugs. That's why I was forced to back out all chain. It's not clear to me what parts you find unacceptable, and what parts you find acceptable. I added you to the reviews in Phabricator from the beginning, so it would have been nice if you could have provided the review feedback there. Since you are slow to provide feedback when I asked you to be a reviewer in Phabricator, but you are quick to revert changes, and provide unclear feedback during your reversion, this makes it very hard to make forward progress in this area. In the past, multiple like Nikos and Martin Matuska have been providing patches in your projects/pf branch, but you don't seem to have the time/interest to push these patches back into head. I've seen that Martin Matuska stopped waiting for you to merge the pf branch to head, and started cherry -- Craig ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Sun, Feb 15, 2015 at 11:46 PM, Gleb Smirnoff gleb...@freebsd.org wrote: On Sun, Feb 15, 2015 at 11:36:17PM -0800, Craig Rodrigues wrote: C On Sun, Feb 15, 2015 at 5:25 PM, Gleb Smirnoff gleb...@freebsd.org wrote: C C On Sun, Feb 15, 2015 at 01:33:19PM -0800, Craig Rodrigues wrote: C C By the way, it would be helpful if you could provide feedback in C C Phabricator. When I created those Phabricator reviews, I added C C you as a reviewer to all of them, so you can't say that you didn't see C C the patches. C C You did not provide feedback on any of them: C C C C https://reviews.freebsd.org/D1309 C C https://reviews.freebsd.org/D1312 C C https://reviews.freebsd.org/D1313 C C https://reviews.freebsd.org/D1315 C C C C Please take some time to go and provide feedback in those C C reviews, so that a better patch can be made that makes you happy. C C Did you address all problems that arised after code was committed? C C Please do, otherwise my review would require me to cut-n-paste from C my own emails. C C C Yes, please cut and paste from your e-mails, and put in the reviews. No, I will not do this. You know right now that you have problems in the aforementioned phab URLs, and you are asking me to look at patches at to point them out to you. This is your task, not mine. Please address all already known problems and update the phab revisions. C It's easier to follow in the individual reviews because there are different C changes C in each review, rather than one big revert, which is what you did. I'm already starting to repeat myself. I did a big revert, because the first change wasn't compilable, later changes fixed compilation failures, but introduced unacceptable bugs. That's why I was forced to back out all chain. It's not clear to me what parts you find unacceptable, and what parts you find acceptable. I added you to the reviews in Phabricator from the beginning, so it would have been nice if you could have provided the review feedback there. Since you are slow to provide feedback when I asked you to be a reviewer in Phabricator, but you are quick to revert changes, and provide unclear feedback during your reversion, this makes it very hard to make forward progress in this area. In the past, multiple like Nikos and Martin Matuska have been providing patches in your projects/pf branch, but you don't seem to have the time/interest to push these patches back into head. I've seen that Martin Matuska stopped waiting for you to merge the pf branch to head, and started cherry -- Craig ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
Craig, On Sat, Feb 14, 2015 at 07:35:53PM -0800, Craig Rodrigues wrote: C On Thu, Jan 22, 2015 at 10:09:41PM +0100, Nikos Vassiliadis wrote: C N Sorry guys, I backed this out due to broken kldunload of pf module, C which C N is critical when you are working with pf bugs. C N C N For sure. 100% understood. C N C N I had to backout r276746 as well, since it has numerous build C breakages, C N that are addressed by later revisions. C N C N That's my fault that I don't review in time, and I will try to improve C N the situation. C N C N Can you please replay r276746 again, addressing all the build problems C N and send the patch to me? You can user reviews.freebsd.org if you C want. C N C N I'd like to get this in, but in a better quality. C N C N I'd like to get involved again and help you fixing pf. Craig could you C N replay 276746? C C I wish you could have fixed the pf unload problem without backing out C all these changes. I took all these changes from your projects/pf branch, C which was starting to bitrot because it was not being sync'd with head. C C I got confirmation from several people that the fixes as they were (after C the build break fixes), C actually fixed their issues with PF and VIMAGE, which have been pending for C several C years now with no visible progress made. C C Most regular users of PF don't really kldunload it once it is used. C For development use, I've been testing inside bhyve VM's, which doesn't C solve the kldunload problem but allows testing and forward progress. C C Why do you want me to replay 276746 and give you a patch? C C Why don't you just do it yourself? Because it is responsibility of the committer, who broke something, to fix it, not mine. Nevertheless, I tried to do that, and it took more than couple of hours, but I failed to untangle all the problems that r276746 brought and proceeded with checkout. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Sun, Feb 15, 2015 at 11:01 AM, Gleb Smirnoff gleb...@freebsd.org wrote: Craig, On Sat, Feb 14, 2015 at 07:35:53PM -0800, Craig Rodrigues wrote: C On Thu, Jan 22, 2015 at 10:09:41PM +0100, Nikos Vassiliadis wrote: C N Sorry guys, I backed this out due to broken kldunload of pf module, C which C N is critical when you are working with pf bugs. C N C N For sure. 100% understood. C N C N I had to backout r276746 as well, since it has numerous build C breakages, C N that are addressed by later revisions. C N C N That's my fault that I don't review in time, and I will try to improve C N the situation. C N C N Can you please replay r276746 again, addressing all the build problems C N and send the patch to me? You can user reviews.freebsd.org if you C want. C N C N I'd like to get this in, but in a better quality. C N C N I'd like to get involved again and help you fixing pf. Craig could you C N replay 276746? C C I wish you could have fixed the pf unload problem without backing out C all these changes. I took all these changes from your projects/pf branch, C which was starting to bitrot because it was not being sync'd with head. C C I got confirmation from several people that the fixes as they were (after C the build break fixes), C actually fixed their issues with PF and VIMAGE, which have been pending for C several C years now with no visible progress made. C C Most regular users of PF don't really kldunload it once it is used. C For development use, I've been testing inside bhyve VM's, which doesn't C solve the kldunload problem but allows testing and forward progress. C C Why do you want me to replay 276746 and give you a patch? C C Why don't you just do it yourself? Because it is responsibility of the committer, who broke something, to fix it, not mine. Nevertheless, I tried to do that, and it took more than couple of hours, but I failed to untangle all the problems that r276746 brought and proceeded with checkout. What are the problems in 276746 that you were unhappy with besides being unable to kldunload PF? With 276746, without the follow-on fixes, if you kldunload PF while VIMAGE is unabled, the kernel panics. That seems worse than being unable to kldunload PF. If I submit another patch, I am just going to commit it, but I don't know if you will be unhappy and revert it. -- Craig ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Sun, Feb 15, 2015 at 1:26 PM, Craig Rodrigues rodr...@freebsd.org wrote: On Sun, Feb 15, 2015 at 11:01 AM, Gleb Smirnoff gleb...@freebsd.org wrote: Craig, On Sat, Feb 14, 2015 at 07:35:53PM -0800, Craig Rodrigues wrote: C On Thu, Jan 22, 2015 at 10:09:41PM +0100, Nikos Vassiliadis wrote: C N Sorry guys, I backed this out due to broken kldunload of pf module, C which C N is critical when you are working with pf bugs. C N C N For sure. 100% understood. C N C N I had to backout r276746 as well, since it has numerous build C breakages, C N that are addressed by later revisions. C N C N That's my fault that I don't review in time, and I will try to improve C N the situation. C N C N Can you please replay r276746 again, addressing all the build problems C N and send the patch to me? You can user reviews.freebsd.org if you C want. C N C N I'd like to get this in, but in a better quality. C N C N I'd like to get involved again and help you fixing pf. Craig could you C N replay 276746? C C I wish you could have fixed the pf unload problem without backing out C all these changes. I took all these changes from your projects/pf branch, C which was starting to bitrot because it was not being sync'd with head. C C I got confirmation from several people that the fixes as they were (after C the build break fixes), C actually fixed their issues with PF and VIMAGE, which have been pending for C several C years now with no visible progress made. C C Most regular users of PF don't really kldunload it once it is used. C For development use, I've been testing inside bhyve VM's, which doesn't C solve the kldunload problem but allows testing and forward progress. C C Why do you want me to replay 276746 and give you a patch? C C Why don't you just do it yourself? Because it is responsibility of the committer, who broke something, to fix it, not mine. Nevertheless, I tried to do that, and it took more than couple of hours, but I failed to untangle all the problems that r276746 brought and proceeded with checkout. What are the problems in 276746 that you were unhappy with besides being unable to kldunload PF? With 276746, without the follow-on fixes, if you kldunload PF while VIMAGE is unabled, the kernel panics. That seems worse than being unable to kldunload PF. If I submit another patch, I am just going to commit it, but I don't know if you will be unhappy and revert it. By the way, it would be helpful if you could provide feedback in Phabricator. When I created those Phabricator reviews, I added you as a reviewer to all of them, so you can't say that you didn't see the patches. You did not provide feedback on any of them: https://reviews.freebsd.org/D1309 https://reviews.freebsd.org/D1312 https://reviews.freebsd.org/D1313 https://reviews.freebsd.org/D1315 Please take some time to go and provide feedback in those reviews, so that a better patch can be made that makes you happy. -- Craig ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Sun, Feb 15, 2015 at 01:33:19PM -0800, Craig Rodrigues wrote: C By the way, it would be helpful if you could provide feedback in C Phabricator. When I created those Phabricator reviews, I added C you as a reviewer to all of them, so you can't say that you didn't see C the patches. C You did not provide feedback on any of them: C C https://reviews.freebsd.org/D1309 C https://reviews.freebsd.org/D1312 C https://reviews.freebsd.org/D1313 C https://reviews.freebsd.org/D1315 C C Please take some time to go and provide feedback in those C reviews, so that a better patch can be made that makes you happy. Did you address all problems that arised after code was committed? Please do, otherwise my review would require me to cut-n-paste from my own emails. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Sun, Feb 15, 2015 at 01:26:18PM -0800, Craig Rodrigues wrote: C What are the problems in 276746 that you were unhappy with C besides being unable to kldunload PF? They were listed in Message-ID: 20150121214445.gl15...@freebsd.org. Also, the fact that commit required a serie of followup commits, means that initial commit was not tested properly. And did I ever said being unable to kldunload PF? This is what I said: You blindly remove kproc_exit(). What do you think would happen on 'kldunload -f pf'? If you can't grok that, you probably don't even understand the code you committed. Since what is going to happen is kernel panic. C With 276746, without the follow-on fixes, if you kldunload PF C while VIMAGE is unabled, the kernel panics. That seems worse C than being unable to kldunload PF. So, without 276746 pf would panic with VIMAGE. And after 276747 pf would panic without VIMAGE. And yes, this is worse. Since backing out only 276747 leads to unbuildable kernel, I ended with backing out all chain of commits. C If I submit another patch, I am just going to commit it, C but I don't know if you will be unhappy and revert it. No, you will wait for review. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Sun, Feb 15, 2015 at 11:36:17PM -0800, Craig Rodrigues wrote: C On Sun, Feb 15, 2015 at 5:25 PM, Gleb Smirnoff gleb...@freebsd.org wrote: C C On Sun, Feb 15, 2015 at 01:33:19PM -0800, Craig Rodrigues wrote: C C By the way, it would be helpful if you could provide feedback in C C Phabricator. When I created those Phabricator reviews, I added C C you as a reviewer to all of them, so you can't say that you didn't see C C the patches. C C You did not provide feedback on any of them: C C C C https://reviews.freebsd.org/D1309 C C https://reviews.freebsd.org/D1312 C C https://reviews.freebsd.org/D1313 C C https://reviews.freebsd.org/D1315 C C C C Please take some time to go and provide feedback in those C C reviews, so that a better patch can be made that makes you happy. C C Did you address all problems that arised after code was committed? C C Please do, otherwise my review would require me to cut-n-paste from C my own emails. C C C Yes, please cut and paste from your e-mails, and put in the reviews. No, I will not do this. You know right now that you have problems in the aforementioned phab URLs, and you are asking me to look at patches at to point them out to you. This is your task, not mine. Please address all already known problems and update the phab revisions. C It's easier to follow in the individual reviews because there are different C changes C in each review, rather than one big revert, which is what you did. I'm already starting to repeat myself. I did a big revert, because the first change wasn't compilable, later changes fixed compilation failures, but introduced unacceptable bugs. That's why I was forced to back out all chain. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Sun, Feb 15, 2015 at 5:25 PM, Gleb Smirnoff gleb...@freebsd.org wrote: On Sun, Feb 15, 2015 at 01:33:19PM -0800, Craig Rodrigues wrote: C By the way, it would be helpful if you could provide feedback in C Phabricator. When I created those Phabricator reviews, I added C you as a reviewer to all of them, so you can't say that you didn't see C the patches. C You did not provide feedback on any of them: C C https://reviews.freebsd.org/D1309 C https://reviews.freebsd.org/D1312 C https://reviews.freebsd.org/D1313 C https://reviews.freebsd.org/D1315 C C Please take some time to go and provide feedback in those C reviews, so that a better patch can be made that makes you happy. Did you address all problems that arised after code was committed? Please do, otherwise my review would require me to cut-n-paste from my own emails. Yes, please cut and paste from your e-mails, and put in the reviews. It's easier to follow in the individual reviews because there are different changes in each review, rather than one big revert, which is what you did. Thanks. -- Craig ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Thu, Jan 22, 2015 at 2:23 PM, Gleb Smirnoff gleb...@freebsd.org wrote: On Thu, Jan 22, 2015 at 10:09:41PM +0100, Nikos Vassiliadis wrote: N Sorry guys, I backed this out due to broken kldunload of pf module, which N is critical when you are working with pf bugs. N N For sure. 100% understood. N N I had to backout r276746 as well, since it has numerous build breakages, N that are addressed by later revisions. N N That's my fault that I don't review in time, and I will try to improve N the situation. N N Can you please replay r276746 again, addressing all the build problems N and send the patch to me? You can user reviews.freebsd.org if you want. N N I'd like to get this in, but in a better quality. N N I'd like to get involved again and help you fixing pf. Craig could you N replay 276746? I wish you could have fixed the pf unload problem without backing out all these changes. I took all these changes from your projects/pf branch, which was starting to bitrot because it was not being sync'd with head. I got confirmation from several people that the fixes as they were (after the build break fixes), actually fixed their issues with PF and VIMAGE, which have been pending for several years now with no visible progress made. Most regular users of PF don't really kldunload it once it is used. For development use, I've been testing inside bhyve VM's, which doesn't solve the kldunload problem but allows testing and forward progress. Why do you want me to replay 276746 and give you a patch? Why don't you just do it yourself? -- Craig ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Thu, Jan 22, 2015 at 10:09:41PM +0100, Nikos Vassiliadis wrote: N Sorry guys, I backed this out due to broken kldunload of pf module, which N is critical when you are working with pf bugs. N N For sure. 100% understood. N N I had to backout r276746 as well, since it has numerous build breakages, N that are addressed by later revisions. N N That's my fault that I don't review in time, and I will try to improve N the situation. N N Can you please replay r276746 again, addressing all the build problems N and send the patch to me? You can user reviews.freebsd.org if you want. N N I'd like to get this in, but in a better quality. N N I'd like to get involved again and help you fixing pf. Craig could you N replay 276746? Erm, I didn't mean replaying 276746 as svn commit. I meant: checkout head, reapply r276746, fix all build breakages, and submit the patch to me. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On 01/22/15 02:27, Gleb Smirnoff wrote: On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: C Author: rodrigc C Date: Tue Jan 6 09:03:03 2015 C New Revision: 276747 C URL: https://svnweb.freebsd.org/changeset/base/276747 C C Log: C Instead of creating a purge thread for every vnet, create C a single purge thread and clean up all vnets from this thread. C C PR: 194515 C Differential Revision: D1315 C Submitted by: Nikos Vassiliadis nv...@gmx.com Sorry guys, I backed this out due to broken kldunload of pf module, which is critical when you are working with pf bugs. For sure. 100% understood. I had to backout r276746 as well, since it has numerous build breakages, that are addressed by later revisions. That's my fault that I don't review in time, and I will try to improve the situation. Can you please replay r276746 again, addressing all the build problems and send the patch to me? You can user reviews.freebsd.org if you want. I'd like to get this in, but in a better quality. I'd like to get involved again and help you fixing pf. Craig could you replay 276746? I can try fixing the pf purge thread. Thanks, Nikos ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Wed, Jan 07, 2015 at 11:46:31PM +0300, Gleb Smirnoff wrote: T On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: T C Author: rodrigc T C Date: Tue Jan 6 09:03:03 2015 T C New Revision: 276747 T C URL: https://svnweb.freebsd.org/changeset/base/276747 T C T C Log: T C Instead of creating a purge thread for every vnet, create T C a single purge thread and clean up all vnets from this thread. T C T C PR: 194515 T C Differential Revision: D1315 T C Submitted by: Nikos Vassiliadis nv...@gmx.com T T I am not sure that this is a good idea. The core idea of VNETs T is that they are isolated from each other. If we serialize purging, T then vnets are strongly affecting each other. T T AFAIU, from the PR there is some panic fixed. What is the actual bug T and why couldn't it be fixed with having per-vnet thread? So, after closer inspection, this commit is a completely messed up. You blindly remove kproc_exit(). What do you think would happen on 'kldunload -f pf'? You removed PF_RULES_RLOCK(). Cool! Now the purging thread doesn't acquire the pf lock. You substitute rw_sleep() with tsleep(). And the latter requires Giant to be held. If you tried your change with INVARIANTS, it would panic immediately. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: C Author: rodrigc C Date: Tue Jan 6 09:03:03 2015 C New Revision: 276747 C URL: https://svnweb.freebsd.org/changeset/base/276747 C C Log: C Instead of creating a purge thread for every vnet, create C a single purge thread and clean up all vnets from this thread. C C PR: 194515 C Differential Revision: D1315 C Submitted by: Nikos Vassiliadis nv...@gmx.com Sorry guys, I backed this out due to broken kldunload of pf module, which is critical when you are working with pf bugs. I had to backout r276746 as well, since it has numerous build breakages, that are addressed by later revisions. That's my fault that I don't review in time, and I will try to improve the situation. Can you please replay r276746 again, addressing all the build problems and send the patch to me? You can user reviews.freebsd.org if you want. I'd like to get this in, but in a better quality. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Thu, Jan 08, 2015 at 12:49:45AM +, Bjoern A. Zeeb wrote: B B AFAIU, from the PR there is some panic fixed. What is the actual bug B B and why couldn't it be fixed with having per-vnet thread? B B B B You don’t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? B B Isn't a vnet, which is a jail, already a set of a dozen of processes? So, B if you are speaking of 3 whatever pf purging threads, then you B already mean “1 mln whatever processes. B B jail/VNETs can exist without a single process attached. B B But I guess the point is that there is only so much work we can do at the same time and we should be very careful in what we try to parallellellellize as with 5 vnets it might be fine, with a couple of thousand you may keep a system busy with itself. Let's admit that thousand of vnets all running pf is bizarre design and has no practical application. B Speaking of pf purging threads competing for resources. If someone wants B really independent pfs in vnets, then locks should be virtualized as well. B B No please don’t. The only places where we “virtualise” locks for VNETs is part of data structures which are vnet specific (virtualised). And the pf state tables (the data the purge threads work on) of course are vnet specific (virtualised). -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Mon, Jan 12, 2015 at 05:41:36PM +0300, Gleb Smirnoff wrote: On Thu, Jan 08, 2015 at 12:49:45AM +, Bjoern A. Zeeb wrote: B B AFAIU, from the PR there is some panic fixed. What is the actual bug B B and why couldn't it be fixed with having per-vnet thread? B B B B You don't 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? B B Isn't a vnet, which is a jail, already a set of a dozen of processes? So, B if you are speaking of 3 whatever pf purging threads, then you B already mean 1 mln whatever processes. B B jail/VNETs can exist without a single process attached. B B But I guess the point is that there is only so much work we can do at the same time and we should be very careful in what we try to parallellellellize as with 5 vnets it might be fine, with a couple of thousand you may keep a system busy with itself. Let's admit that thousand of vnets all running pf is bizarre design and has no practical application. Hosted firewall/NAT for ISP/Data centers. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On 12 January 2015 at 08:05, Slawa Olhovchenkov s...@zxy.spb.ru wrote: On Mon, Jan 12, 2015 at 05:41:36PM +0300, Gleb Smirnoff wrote: On Thu, Jan 08, 2015 at 12:49:45AM +, Bjoern A. Zeeb wrote: B B AFAIU, from the PR there is some panic fixed. What is the actual bug B B and why couldn't it be fixed with having per-vnet thread? B B B B You don't 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? B B Isn't a vnet, which is a jail, already a set of a dozen of processes? So, B if you are speaking of 3 whatever pf purging threads, then you B already mean 1 mln whatever processes. B B jail/VNETs can exist without a single process attached. B B But I guess the point is that there is only so much work we can do at the same time and we should be very careful in what we try to parallellellellize as with 5 vnets it might be fine, with a couple of thousand you may keep a system busy with itself. Let's admit that thousand of vnets all running pf is bizarre design and has no practical application. Hosted firewall/NAT for ISP/Data centers. Then let's bite the bullet and setup some per-something (maybe CPU, maybe RSS, etc) global taskqueues to run cooperative multitasked bits like this on. This isn't the only thing that we'll want to have potentially tens of thousands of, but not have tens of thousands of worker threads. -adrian ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Thu, Jan 8, 2015 at 1:21 AM, Bjoern A. Zeeb bzeeb-li...@lists.zabbadoz.net wrote: On 07 Jan 2015, at 20:46 , Gleb Smirnoff gleb...@freebsd.org wrote: On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: C Author: rodrigc C Date: Tue Jan 6 09:03:03 2015 C New Revision: 276747 C URL: https://svnweb.freebsd.org/changeset/base/276747 C C Log: C Instead of creating a purge thread for every vnet, create C a single purge thread and clean up all vnets from this thread. C C PR: 194515 C Differential Revision: D1315 C Submitted by: Nikos Vassiliadis nv...@gmx.com I am not sure that this is a good idea. The core idea of VNETs is that they are isolated from each other. If we serialize purging, then vnets are strongly affecting each other. AFAIU, from the PR there is some panic fixed. What is the actual bug and why couldn't it be fixed with having per-vnet thread? You don’t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? You can tune your system to your load! I do not agree with this change as well but just saw it! I would have agreed with this if a thread per CPU is created and some improvements in the locking strategy is performed! This is a potential issue since on busy system this thread gets very resource consuming! — Bjoern A. Zeeb Charles Haddon Spurgeon: Friendship is one of the sweetest joys of life. Many might have failed beneath the bitterness of their trial had they not found a friend. -- Ermal ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On 8 January 2015 at 00:13, Ermal Luçi e...@freebsd.org wrote: On Thu, Jan 8, 2015 at 1:21 AM, Bjoern A. Zeeb bzeeb-li...@lists.zabbadoz.net wrote: On 07 Jan 2015, at 20:46 , Gleb Smirnoff gleb...@freebsd.org wrote: On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: C Author: rodrigc C Date: Tue Jan 6 09:03:03 2015 C New Revision: 276747 C URL: https://svnweb.freebsd.org/changeset/base/276747 C C Log: C Instead of creating a purge thread for every vnet, create C a single purge thread and clean up all vnets from this thread. C C PR: 194515 C Differential Revision: D1315 C Submitted by: Nikos Vassiliadis nv...@gmx.com I am not sure that this is a good idea. The core idea of VNETs is that they are isolated from each other. If we serialize purging, then vnets are strongly affecting each other. AFAIU, from the PR there is some panic fixed. What is the actual bug and why couldn't it be fixed with having per-vnet thread? You don’t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? You can tune your system to your load! I do not agree with this change as well but just saw it! I would have agreed with this if a thread per CPU is created and some improvements in the locking strategy is performed! This is a potential issue since on busy system this thread gets very resource consuming! So the tricksy bit here is once you have things being called via a taskqueue, they're effectively cooperative multitasking bits. A lot of things I've found aren't .. really designed for this. So I'm all for it, and I think it's a good idea in general, but then the pieces need to be reviewed for their suitability for this and may need some reworking so they don't hog CPU. (Yes, it's like writing WIN16 or MACOS code, or maybe network drivers in FreeBSD, but you get the idea.) -adrian ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
Julian Elischer wrote this message on Thu, Jan 08, 2015 at 11:37 +0800: On 1/8/15 8:31 AM, Gleb Smirnoff wrote: On Thu, Jan 08, 2015 at 12:21:57AM +, Bjoern A. Zeeb wrote: B B On 07 Jan 2015, at 20:46 , Gleb Smirnoff gleb...@freebsd.org wrote: B B On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: B C Author: rodrigc B C Date: Tue Jan 6 09:03:03 2015 B C New Revision: 276747 B C URL: https://svnweb.freebsd.org/changeset/base/276747 B C B C Log: B C Instead of creating a purge thread for every vnet, create B C a single purge thread and clean up all vnets from this thread. B C B C PR: 194515 B C Differential Revision: D1315 B C Submitted by: Nikos Vassiliadis nv...@gmx.com B B I am not sure that this is a good idea. The core idea of VNETs B is that they are isolated from each other. If we serialize purging, B then vnets are strongly affecting each other. B B AFAIU, from the PR there is some panic fixed. What is the actual bug B and why couldn't it be fixed with having per-vnet thread? B B You don???t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? Isn't a vnet, which is a jail, already a set of a dozen of processes? So, if you are speaking of 3 whatever pf purging threads, then you already mean 1 mln whatever processes. Actually, no. as we have presetned it, a vnet is part of a jail. But, it was originally an independnent thing, like FIBS, and a jail may exist with a single process. I think one should be enough.. or if that it is not sufficient, then at maximum, one per cpu We really need to make a library that handles creating/scheduling things like these better so people aren't reinventing them over and over again.. We do this in geli for creating threads for each geli worker... Can't something like taskqueue_start_threads_pinned be used for this? -- John-Mark Gurney Voice: +1 415 225 5579 All that I will do, has been done, All that I have, has not. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On 1/8/15 8:31 AM, Gleb Smirnoff wrote: On Thu, Jan 08, 2015 at 12:21:57AM +, Bjoern A. Zeeb wrote: B B On 07 Jan 2015, at 20:46 , Gleb Smirnoff gleb...@freebsd.org wrote: B B On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: B C Author: rodrigc B C Date: Tue Jan 6 09:03:03 2015 B C New Revision: 276747 B C URL: https://svnweb.freebsd.org/changeset/base/276747 B C B C Log: B C Instead of creating a purge thread for every vnet, create B C a single purge thread and clean up all vnets from this thread. B C B C PR: 194515 B C Differential Revision: D1315 B C Submitted by: Nikos Vassiliadis nv...@gmx.com B B I am not sure that this is a good idea. The core idea of VNETs B is that they are isolated from each other. If we serialize purging, B then vnets are strongly affecting each other. B B AFAIU, from the PR there is some panic fixed. What is the actual bug B and why couldn't it be fixed with having per-vnet thread? B B You don’t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? Isn't a vnet, which is a jail, already a set of a dozen of processes? So, if you are speaking of 3 whatever pf purging threads, then you already mean 1 mln whatever processes. Actually, no. as we have presetned it, a vnet is part of a jail. But, it was originally an independnent thing, like FIBS, and a jail may exist with a single process. I think one should be enough.. or if that it is not sufficient, then at maximum, one per cpu Speaking of pf purging threads competing for resources. If someone wants really independent pfs in vnets, then locks should be virtualized as well. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On 07 Jan 2015, at 20:46 , Gleb Smirnoff gleb...@freebsd.org wrote: On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: C Author: rodrigc C Date: Tue Jan 6 09:03:03 2015 C New Revision: 276747 C URL: https://svnweb.freebsd.org/changeset/base/276747 C C Log: C Instead of creating a purge thread for every vnet, create C a single purge thread and clean up all vnets from this thread. C C PR: 194515 C Differential Revision: D1315 C Submitted by: Nikos Vassiliadis nv...@gmx.com I am not sure that this is a good idea. The core idea of VNETs is that they are isolated from each other. If we serialize purging, then vnets are strongly affecting each other. AFAIU, from the PR there is some panic fixed. What is the actual bug and why couldn't it be fixed with having per-vnet thread? You don’t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? — Bjoern A. Zeeb Charles Haddon Spurgeon: Friendship is one of the sweetest joys of life. Many might have failed beneath the bitterness of their trial had they not found a friend. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Thu, Jan 08, 2015 at 12:21:57AM +, Bjoern A. Zeeb wrote: B B On 07 Jan 2015, at 20:46 , Gleb Smirnoff gleb...@freebsd.org wrote: B B On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: B C Author: rodrigc B C Date: Tue Jan 6 09:03:03 2015 B C New Revision: 276747 B C URL: https://svnweb.freebsd.org/changeset/base/276747 B C B C Log: B C Instead of creating a purge thread for every vnet, create B C a single purge thread and clean up all vnets from this thread. B C B C PR: 194515 B C Differential Revision: D1315 B C Submitted by: Nikos Vassiliadis nv...@gmx.com B B I am not sure that this is a good idea. The core idea of VNETs B is that they are isolated from each other. If we serialize purging, B then vnets are strongly affecting each other. B B AFAIU, from the PR there is some panic fixed. What is the actual bug B and why couldn't it be fixed with having per-vnet thread? B B You don’t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? Isn't a vnet, which is a jail, already a set of a dozen of processes? So, if you are speaking of 3 whatever pf purging threads, then you already mean 1 mln whatever processes. Speaking of pf purging threads competing for resources. If someone wants really independent pfs in vnets, then locks should be virtualized as well. -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On 08 Jan 2015, at 00:31 , Gleb Smirnoff gleb...@freebsd.org wrote: On Thu, Jan 08, 2015 at 12:21:57AM +, Bjoern A. Zeeb wrote: B B On 07 Jan 2015, at 20:46 , Gleb Smirnoff gleb...@freebsd.org wrote: B B On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: B C Author: rodrigc B C Date: Tue Jan 6 09:03:03 2015 B C New Revision: 276747 B C URL: https://svnweb.freebsd.org/changeset/base/276747 B C B C Log: B C Instead of creating a purge thread for every vnet, create B C a single purge thread and clean up all vnets from this thread. B C B C PR: 194515 B C Differential Revision: D1315 B C Submitted by: Nikos Vassiliadis nv...@gmx.com B B I am not sure that this is a good idea. The core idea of VNETs B is that they are isolated from each other. If we serialize purging, B then vnets are strongly affecting each other. B B AFAIU, from the PR there is some panic fixed. What is the actual bug B and why couldn't it be fixed with having per-vnet thread? B B You don’t 3 whatever pf purging threads on a system all running, possibly competing for some resources, e.g., locks? Isn't a vnet, which is a jail, already a set of a dozen of processes? So, if you are speaking of 3 whatever pf purging threads, then you already mean “1 mln whatever processes. jail/VNETs can exist without a single process attached. But I guess the point is that there is only so much work we can do at the same time and we should be very careful in what we try to parallellellellize as with 5 vnets it might be fine, with a couple of thousand you may keep a system busy with itself. Speaking of pf purging threads competing for resources. If someone wants really independent pfs in vnets, then locks should be virtualized as well. No please don’t. The only places where we “virtualise” locks for VNETs is part of data structures which are vnet specific (virtualised). — Bjoern A. Zeeb Charles Haddon Spurgeon: Friendship is one of the sweetest joys of life. Many might have failed beneath the bitterness of their trial had they not found a friend. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: C Author: rodrigc C Date: Tue Jan 6 09:03:03 2015 C New Revision: 276747 C URL: https://svnweb.freebsd.org/changeset/base/276747 C C Log: C Instead of creating a purge thread for every vnet, create C a single purge thread and clean up all vnets from this thread. C C PR: 194515 C Differential Revision: D1315 C Submitted by: Nikos Vassiliadis nv...@gmx.com I am not sure that this is a good idea. The core idea of VNETs is that they are isolated from each other. If we serialize purging, then vnets are strongly affecting each other. AFAIU, from the PR there is some panic fixed. What is the actual bug and why couldn't it be fixed with having per-vnet thread? -- Totus tuus, Glebius. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
Hi, On 01/07/15 21:46, Gleb Smirnoff wrote: On Tue, Jan 06, 2015 at 09:03:04AM +, Craig Rodrigues wrote: C Author: rodrigc C Date: Tue Jan 6 09:03:03 2015 C New Revision: 276747 C URL: https://svnweb.freebsd.org/changeset/base/276747 C C Log: C Instead of creating a purge thread for every vnet, create C a single purge thread and clean up all vnets from this thread. C C PR: 194515 C Differential Revision: D1315 C Submitted by: Nikos Vassiliadis nv...@gmx.com I am not sure that this is a good idea. The core idea of VNETs is that they are isolated from each other. If we serialize purging, then vnets are strongly affecting each other. That is true. AFAIU, from the PR there is some panic fixed. What is the actual bug and why couldn't it be fixed with having per-vnet thread? I don't remember the exact bug, this was written in summer of 2013, but the code that creates the threads seemed complex. The decision to use one thread was based on the fact that a lot of resources are used when a big number (100 let's say) of vnets is active. Purging already runs 10 times a second. Something between those two is better. Maybe, creating threads up to a certain number and above that map new vnets to existing threads? Nikos ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r276747 - head/sys/netpfil/pf
On 01/06/15 09:39, Craig Rodrigues wrote: Author: rodrigc Date: Tue Jan 6 08:39:06 2015 New Revision: 276746 URL: https://svnweb.freebsd.org/changeset/base/276746 Log: Merge: r258322 from projects/pf branch Split functions that initialize various pf parts into their vimage parts and global parts. Since global parts appeared to be only mutex initializations, just abandon them and use MTX_SYSINIT() instead. Kill my incorrect VNET_FOREACH() iterator and instead use correct approach with VNET_SYSINIT(). PR: 194515 Differential Revision: D1309 Submitted by: glebius, Nikos Vassiliadis nv...@gmx.com Reviewed by:trociny, zec, gnn This results in the following build failures: sys/modules/pf/../../netpfil/pf/pf_ioctl.c:3728:3: error: use of undeclared identifier 'V_pf_end_threads' V_pf_end_threads = 0; ^ sys/modules/pf/../../netpfil/pf/pf_ioctl.c:3815:6: error: use of undeclared identifier 'vnet_pf_init' vnet_pf_init, NULL); ^ sys/net/vnet.h:414:35: note: expanded from macro 'VNET_SYSINIT' SYSINIT(ident, subsystem, order, func, arg) ^ sys/sys/kernel.h:243:36: note: expanded from macro 'SYSINIT' (sysinit_cfunc_t)(sysinit_nfunc_t)func, (void *)(ident)) ^ sys/sys/kernel.h:236:3: note: expanded from macro 'C_SYSINIT' func --HPS ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
