Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 21 Oct 2011 20:57:47 +0200
From:      =?ISO-8859-1?Q?Ermal_Lu=E7i?= <eri@freebsd.org>
To:        "Bjoern A. Zeeb" <bz@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r226536 - head/sys/contrib/pf/net
Message-ID:  <CAPBZQG3FyJzXuMdv2aiUJkU6yXq7KYG59vZ0yoLii%2BrL2EEtDw@mail.gmail.com>
In-Reply-To: <201110191104.p9JB4nlK021378@svn.freebsd.org>
References:  <201110191104.p9JB4nlK021378@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help 
On Wed, Oct 19, 2011 at 1:04 PM, Bjoern A. Zeeb <bz@freebsd.org> wrote:
> Author: bz
> Date: Wed Oct 19 11:04:49 2011
> New Revision: 226536
> URL: http://svn.freebsd.org/changeset/base/226536
>
> Log:
> =A0De-virtualize the pf_task_mtx lock. =A0At the current state of pf lock=
ing
> =A0and virtualization it is not helpful but complicates things.

I would disagree with this since its a step backwards and different
direction with pf(4) code in general.
The patch to actually fix it for vimage enabled kernels was simpler!


>
> =A0Current state of art is to not virtualize these kinds of locks -
> =A0inp_group/hash/info/.. are all not virtualized either.
>
> =A0MFC after: =A0 =A03 days
>
> Modified:
> =A0head/sys/contrib/pf/net/pf_ioctl.c
> =A0head/sys/contrib/pf/net/pfvar.h
>
> Modified: head/sys/contrib/pf/net/pf_ioctl.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- head/sys/contrib/pf/net/pf_ioctl.c =A0Wed Oct 19 10:16:42 2011 =A0 =
=A0 =A0 =A0(r226535)
> +++ head/sys/contrib/pf/net/pf_ioctl.c =A0Wed Oct 19 11:04:49 2011 =A0 =
=A0 =A0 =A0(r226536)
> @@ -266,7 +266,7 @@ static struct cdevsw pf_cdevsw =3D {
> =A0static volatile VNET_DEFINE(int, pf_pfil_hooked);
> =A0#define V_pf_pfil_hooked =A0 =A0 =A0 VNET(pf_pfil_hooked)
> =A0VNET_DEFINE(int, =A0 =A0 =A0 =A0 =A0 =A0 =A0 pf_end_threads);
> -VNET_DEFINE(struct mtx, =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pf_task_mtx);
> +struct mtx =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pf_task_mtx;
>
> =A0/* pfsync */
> =A0pfsync_state_import_t =A0 =A0 =A0 =A0 =A0*pfsync_state_import_ptr =3D =
NULL;
> @@ -287,18 +287,18 @@ SYSCTL_VNET_INT(_debug, OID_AUTO, pfugid
> =A0 =A0 =A0 =A0&VNET_NAME(debug_pfugidhack), 0,
> =A0 =A0 =A0 =A0"Enable/disable pf user/group rules mpsafe hack");
>
> -void
> +static void
> =A0init_pf_mutex(void)
> =A0{
>
> - =A0 =A0 =A0 mtx_init(&V_pf_task_mtx, "pf task mtx", NULL, MTX_DEF);
> + =A0 =A0 =A0 mtx_init(&pf_task_mtx, "pf task mtx", NULL, MTX_DEF);
> =A0}
>
> -void
> +static void
> =A0destroy_pf_mutex(void)
> =A0{
>
> - =A0 =A0 =A0 mtx_destroy(&V_pf_task_mtx);
> + =A0 =A0 =A0 mtx_destroy(&pf_task_mtx);
> =A0}
> =A0void
> =A0init_zone_var(void)
> @@ -4381,11 +4381,8 @@ pf_load(void)
>
> =A0 =A0 =A0 =A0init_zone_var();
> =A0 =A0 =A0 =A0sx_init(&V_pf_consistency_lock, "pf_statetbl_lock");
> - =A0 =A0 =A0 init_pf_mutex();
> - =A0 =A0 =A0 if (pfattach() < 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 destroy_pf_mutex();
> + =A0 =A0 =A0 if (pfattach() < 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return (ENOMEM);
> - =A0 =A0 =A0 }
>
> =A0 =A0 =A0 =A0return (0);
> =A0}
> @@ -4413,14 +4410,13 @@ pf_unload(void)
> =A0 =A0 =A0 =A0V_pf_end_threads =3D 1;
> =A0 =A0 =A0 =A0while (V_pf_end_threads < 2) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0wakeup_one(pf_purge_thread);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 msleep(pf_purge_thread, &V_pf_task_mtx, 0, =
"pftmo", hz);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 msleep(pf_purge_thread, &pf_task_mtx, 0, "p=
ftmo", hz);
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0pfi_cleanup();
> =A0 =A0 =A0 =A0pf_osfp_flush();
> =A0 =A0 =A0 =A0pf_osfp_cleanup();
> =A0 =A0 =A0 =A0cleanup_pf_zone();
> =A0 =A0 =A0 =A0PF_UNLOCK();
> - =A0 =A0 =A0 destroy_pf_mutex();
> =A0 =A0 =A0 =A0sx_destroy(&V_pf_consistency_lock);
> =A0 =A0 =A0 =A0return error;
> =A0}
> @@ -4432,10 +4428,12 @@ pf_modevent(module_t mod, int type, void
>
> =A0 =A0 =A0 =A0switch(type) {
> =A0 =A0 =A0 =A0case MOD_LOAD:
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 init_pf_mutex();
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pf_dev =3D make_dev(&pf_cdevsw, 0, 0, 0, 0=
600, PF_NAME);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0case MOD_UNLOAD:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0destroy_dev(pf_dev);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 destroy_pf_mutex();
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0default:
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error =3D EINVAL;
>
> Modified: head/sys/contrib/pf/net/pfvar.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- head/sys/contrib/pf/net/pfvar.h =A0 =A0 Wed Oct 19 10:16:42 2011 =A0 =
=A0 =A0 =A0(r226535)
> +++ head/sys/contrib/pf/net/pfvar.h =A0 =A0 Wed Oct 19 11:04:49 2011 =A0 =
=A0 =A0 =A0(r226536)
> @@ -237,19 +237,18 @@ struct pfi_dynaddr {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uma_zdestroy(var)
>
> =A0#ifdef __FreeBSD__
> -VNET_DECLARE(struct mtx, =A0 =A0 =A0 =A0pf_task_mtx);
> -#define =A0 =A0 =A0 =A0V_pf_task_mtx =A0 =A0 =A0 =A0 =A0 =A0VNET(pf_task=
_mtx)
> +extern struct mtx pf_task_mtx;
>
> -#define =A0 =A0 =A0 =A0PF_LOCK_ASSERT() =A0 =A0 =A0 =A0mtx_assert(&V_pf_=
task_mtx, MA_OWNED)
> -#define =A0 =A0 =A0 =A0PF_UNLOCK_ASSERT() =A0 =A0 =A0mtx_assert(&V_pf_ta=
sk_mtx, MA_NOTOWNED)
> +#define =A0 =A0 =A0 =A0PF_LOCK_ASSERT() =A0 =A0 =A0 =A0mtx_assert(&pf_ta=
sk_mtx, MA_OWNED)
> +#define =A0 =A0 =A0 =A0PF_UNLOCK_ASSERT() =A0 =A0 =A0mtx_assert(&pf_task=
_mtx, MA_NOTOWNED)
>
> =A0#define =A0 =A0 =A0 =A0PF_LOCK() =A0 =A0 =A0 do { =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
> =A0 =A0 =A0 =A0PF_UNLOCK_ASSERT(); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 \
> - =A0 =A0 =A0 mtx_lock(&V_pf_task_mtx); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 \
> + =A0 =A0 =A0 mtx_lock(&pf_task_mtx); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 \
> =A0} while(0)
> =A0#define =A0 =A0 =A0 =A0PF_UNLOCK() =A0 =A0 do { =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
> =A0 =A0 =A0 =A0PF_LOCK_ASSERT(); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 \
> - =A0 =A0 =A0 mtx_unlock(&V_pf_task_mtx); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 \
> + =A0 =A0 =A0 mtx_unlock(&pf_task_mtx); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 \
> =A0} while(0)
> =A0#else
> =A0#define =A0 =A0 =A0 =A0PF_LOCK_ASSERT()
> @@ -270,9 +269,6 @@ VNET_DECLARE(struct mtx, =A0 =A0 pf_task_mtx);
> =A0 =A0 =A0 =A0PF_LOCK(); =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
> =A0} while(0)
>
> -extern void init_pf_mutex(void);
> -extern void destroy_pf_mutex(void);
> -
> =A0#define =A0 =A0 =A0 =A0PF_MODVER =A0 =A0 =A0 1
> =A0#define =A0 =A0 =A0 =A0PFLOG_MODVER =A0 =A01
> =A0#define =A0 =A0 =A0 =A0PFSYNC_MODVER =A0 1
>



--=20
Ermal

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPBZQG3FyJzXuMdv2aiUJkU6yXq7KYG59vZ0yoLii%2BrL2EEtDw>