All of lore.kernel.org
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Vlad Yasevich <vyasevich@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	jan.ariyasu@gmail.com, linux-sctp@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	jan.ariyasu@hp.com
Subject: Re: [PATCH 00/13] SCTP: Enable netns
Date: Mon, 06 Aug 2012 23:06:02 +0000	[thread overview]
Message-ID: <87txwfipt1.fsf@xmission.com> (raw)
In-Reply-To: <502039A8.7080807@gmail.com> (Vlad Yasevich's message of "Mon, 06 Aug 2012 17:39:52 -0400")

Vlad Yasevich <vyasevich@gmail.com> writes:

> On 08/06/2012 04:47 PM, David Miller wrote:
>> From: ebiederm@xmission.com (Eric W. Biederman)
>> Date: Mon, 06 Aug 2012 12:50:46 -0700
>>
>>> Vlad Yasevich <vyasevich@gmail.com> writes:
>>>
>>>
>>>> Hi Eric
>>>>
>>>> Associations are looked up by ports, but then verifyed by addresses.
>>>> Also, associations belong to sockets and simply validating the socket
>>>> namespace should be sufficient.
>>>
>>> True.  Your set of patches isn't quite as likely to malfunction as it
>>> looked at first glance.  It requires address reuse which happens accross
>>> namespaces but not too frequently.
>>>
>>> As for validating the socket namespace I agree that is the fix and my
>>> patchset winds up doing it.
>>
>> FWIW I much prefer Eric's patch set, it was so significantly easier to
>> read and validate than Jan's.
>>
>
> Yes, but Eric's patch set is missing a very significant piece which is per-net
> sctp tunables/globals.  I think adding that piece in will introduce some of the
> complexities of Jan's patch.

And Jan's patchset is missing the per net snmp counters, as well
as the per net checks when looking in the hash tables.  Things that look
like fairly fundamental correctness issues.

No offense but I think Jan's patchset has minor but significant
structural flaws and correctness issues that would require the patchset
to be respun, after review even if my patches did not exist.

The poor movement of variables from their global context into the per
net context is one of those problems.

The fact that after Jan's patchset all of the tunables despite being
made per net remain in struct sctp_globals and sit their unused is a
recipe for confusion.

So I think the simplest path forward would be to merge my patches, and
then merge Jan's tunable work after she has had a chance to rebase it.

> Also, I noticed that Eric went the route of placing sctp netns into struct net,
> but Jan used a generic pointer.  Which one should be used? Is there some
> guidance?

Placing things directly in struct net is more efficient.  Using net
generic is more convenient for development as using net generic does not
require a kernel recompile.

For minor things and especially where it does not have an impact on
performance net generic is the way to go.

It looked to me that sctp was fundamental enough and had enough
important data structures on the hot path that placing the variables
in struct net directly seemed sensible.

Eric

WARNING: multiple messages have this Message-ID (diff)
From: ebiederm@xmission.com (Eric W. Biederman)
To: Vlad Yasevich <vyasevich@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	jan.ariyasu@gmail.com, linux-sctp@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	jan.ariyasu@hp.com
Subject: Re: [PATCH 00/13] SCTP: Enable netns
Date: Mon, 06 Aug 2012 16:06:02 -0700	[thread overview]
Message-ID: <87txwfipt1.fsf@xmission.com> (raw)
In-Reply-To: <502039A8.7080807@gmail.com> (Vlad Yasevich's message of "Mon, 06 Aug 2012 17:39:52 -0400")

Vlad Yasevich <vyasevich@gmail.com> writes:

> On 08/06/2012 04:47 PM, David Miller wrote:
>> From: ebiederm@xmission.com (Eric W. Biederman)
>> Date: Mon, 06 Aug 2012 12:50:46 -0700
>>
>>> Vlad Yasevich <vyasevich@gmail.com> writes:
>>>
>>>
>>>> Hi Eric
>>>>
>>>> Associations are looked up by ports, but then verifyed by addresses.
>>>> Also, associations belong to sockets and simply validating the socket
>>>> namespace should be sufficient.
>>>
>>> True.  Your set of patches isn't quite as likely to malfunction as it
>>> looked at first glance.  It requires address reuse which happens accross
>>> namespaces but not too frequently.
>>>
>>> As for validating the socket namespace I agree that is the fix and my
>>> patchset winds up doing it.
>>
>> FWIW I much prefer Eric's patch set, it was so significantly easier to
>> read and validate than Jan's.
>>
>
> Yes, but Eric's patch set is missing a very significant piece which is per-net
> sctp tunables/globals.  I think adding that piece in will introduce some of the
> complexities of Jan's patch.

And Jan's patchset is missing the per net snmp counters, as well
as the per net checks when looking in the hash tables.  Things that look
like fairly fundamental correctness issues.

No offense but I think Jan's patchset has minor but significant
structural flaws and correctness issues that would require the patchset
to be respun, after review even if my patches did not exist.

The poor movement of variables from their global context into the per
net context is one of those problems.

The fact that after Jan's patchset all of the tunables despite being
made per net remain in struct sctp_globals and sit their unused is a
recipe for confusion.

So I think the simplest path forward would be to merge my patches, and
then merge Jan's tunable work after she has had a chance to rebase it.

> Also, I noticed that Eric went the route of placing sctp netns into struct net,
> but Jan used a generic pointer.  Which one should be used? Is there some
> guidance?

Placing things directly in struct net is more efficient.  Using net
generic is more convenient for development as using net generic does not
require a kernel recompile.

For minor things and especially where it does not have an impact on
performance net generic is the way to go.

It looked to me that sctp was fundamental enough and had enough
important data structures on the hot path that placing the variables
in struct net directly seemed sensible.

Eric

  reply	other threads:[~2012-08-06 23:06 UTC|newest]

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-04 21:30 [PATCH 00/13] SCTP: Enable netns Jan Ariyasu
2012-08-04 21:30 ` Jan Ariyasu
2012-08-04 23:57 ` David Miller
2012-08-04 23:57   ` David Miller
2012-08-06 18:20 ` Eric W. Biederman
2012-08-06 18:20   ` Eric W. Biederman
2012-08-06 18:38   ` [PATCH net-next 0/9] sctp: Basic support for network namespaces Eric W. Biederman
2012-08-06 18:38     ` Eric W. Biederman
2012-08-06 18:39     ` [PATCH net-next 1/9] sctp: Make the port hash table use struct net in it's key Eric W. Biederman
2012-08-06 18:39       ` Eric W. Biederman
2012-08-15  3:18       ` Vlad Yasevich
2012-08-15  3:18         ` Vlad Yasevich
2012-08-06 18:40     ` [PATCH net-next 2/9] sctp: Make the endpoint hashtable handle multiple network namespaces Eric W. Biederman
2012-08-06 18:40       ` Eric W. Biederman
2012-08-15  3:18       ` Vlad Yasevich
2012-08-15  3:18         ` Vlad Yasevich
2012-08-06 18:41     ` [PATCH net-next 3/9] sctp: Make the association " Eric W. Biederman
2012-08-06 18:41       ` Eric W. Biederman
2012-08-15  3:18       ` Vlad Yasevich
2012-08-15  3:18         ` Vlad Yasevich
2012-08-06 18:42     ` [PATCH net-next 4/9] sctp: Make the address lists per network namespace Eric W. Biederman
2012-08-06 18:42       ` Eric W. Biederman
2012-08-15  3:19       ` Vlad Yasevich
2012-08-15  3:19         ` Vlad Yasevich
2012-08-06 18:43     ` [PATCH net-next 5/9] sctp: Make the ctl_sock " Eric W. Biederman
2012-08-06 18:43       ` Eric W. Biederman
2012-08-15  3:19       ` Vlad Yasevich
2012-08-15  3:19         ` Vlad Yasevich
2012-08-06 18:44     ` [PATCH net-next 6/9] sctp: Move the percpu sockets counter out of sctp_proc_init Eric W. Biederman
2012-08-06 18:44       ` Eric W. Biederman
2012-08-15  3:19       ` Vlad Yasevich
2012-08-15  3:19         ` Vlad Yasevich
2012-08-06 18:45     ` [PATCH net-next 7/9] sctp: Make the proc files per network namespace Eric W. Biederman
2012-08-06 18:45       ` Eric W. Biederman
2012-08-15  3:19       ` Vlad Yasevich
2012-08-15  3:19         ` Vlad Yasevich
2012-08-06 18:46     ` [PATCH net-next 8/9] sctp: Enable sctp in all network namespaces Eric W. Biederman
2012-08-06 18:46       ` Eric W. Biederman
2012-08-15  3:20       ` Vlad Yasevich
2012-08-15  3:20         ` Vlad Yasevich
2012-08-06 18:47     ` [PATCH net-next 9/9] sctp: Make the mib per network namespace Eric W. Biederman
2012-08-06 18:47       ` Eric W. Biederman
2012-08-15  3:20       ` Vlad Yasevich
2012-08-15  3:20         ` Vlad Yasevich
2012-08-07 17:17     ` [PATCH net-next 0/7] sctp: network namespace support Part 2: per net tunables Eric W. Biederman
2012-08-07 17:17       ` Eric W. Biederman
2012-08-07 17:23       ` [PATCH net-next 1/7] sctp: Add infrastructure for per net sysctls Eric W. Biederman
2012-08-07 17:23         ` Eric W. Biederman
2012-08-15  3:20         ` Vlad Yasevich
2012-08-15  3:20           ` Vlad Yasevich
2012-08-07 17:25       ` [PATCH net-next 2/7] sctp: Push struct net down to sctp_chunk_event_lookup Eric W. Biederman
2012-08-07 17:25         ` Eric W. Biederman
2012-08-07 17:26       ` [PATCH net-next 3/7] sctp: Push struct net down into sctp_transport_init Eric W. Biederman
2012-08-07 17:26         ` Eric W. Biederman
2012-08-07 17:27       ` [PATCH net-next 4/7] sctp: Push struct net down into sctp_in_scope Eric W. Biederman
2012-08-07 17:27         ` Eric W. Biederman
2012-08-07 17:28       ` [PATCH net-next 5/7] sctp: Push struct net down into all of the state machine functions Eric W. Biederman
2012-08-07 17:29       ` [PATCH net-next 6/7] sctp: Push struct net down into sctp_verify_ext_param Eric W. Biederman
2012-08-07 17:29         ` Eric W. Biederman
2012-08-07 17:29       ` [PATCH net-next 7/7] sctp: Make sysctl tunables per net Eric W. Biederman
2012-08-07 17:29         ` Eric W. Biederman
2012-08-09  6:20       ` [PATCH net-next 0/7] sctp: network namespace support Part 2: per net tunables David Miller
2012-08-09  6:20         ` David Miller
2012-08-09 14:07         ` Vlad Yasevich
2012-08-09 14:07           ` Vlad Yasevich
2012-08-14 21:14         ` David Miller
2012-08-14 21:14           ` David Miller
2012-08-15  3:16           ` Vlad Yasevich
2012-08-15  3:16             ` Vlad Yasevich
2012-08-15  3:21       ` Vlad Yasevich
2012-08-15  3:21         ` Vlad Yasevich
2012-08-15  6:10       ` David Miller
2012-08-15  6:10         ` David Miller
2012-08-06 19:21   ` [PATCH 00/13] SCTP: Enable netns Vlad Yasevich
2012-08-06 19:21     ` Vlad Yasevich
2012-08-06 19:50     ` Eric W. Biederman
2012-08-06 19:50       ` Eric W. Biederman
2012-08-06 20:06       ` Vlad Yasevich
2012-08-06 20:06         ` Vlad Yasevich
2012-08-06 20:47       ` David Miller
2012-08-06 20:47         ` David Miller
2012-08-06 21:39         ` Vlad Yasevich
2012-08-06 21:39           ` Vlad Yasevich
2012-08-06 23:06           ` Eric W. Biederman [this message]
2012-08-06 23:06             ` Eric W. Biederman
2012-08-15  3:23 ` Vlad Yasevich
2012-08-15  3:23   ` Vlad Yasevich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87txwfipt1.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=davem@davemloft.net \
    --cc=jan.ariyasu@gmail.com \
    --cc=jan.ariyasu@hp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sctp@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vyasevich@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.