All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jamal Hadi Salim <jhs@mojatatu.com>
To: Cong Wang <xiyou.wangcong@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>
Subject: Re: [Patch net-next] net_sched: add network namespace support for tc actions
Date: Mon, 22 Feb 2016 08:42:43 -0500	[thread overview]
Message-ID: <56CB1053.8020102@mojatatu.com> (raw)
In-Reply-To: <CAM_iQpWzVoy7eYdf2Yxz2qfoAO5-pDZioACyu7Hws6u7vvP1Yw@mail.gmail.com>

On 16-02-20 07:46 PM, Cong Wang wrote:
> On Sat, Feb 20, 2016 at 10:36 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Do you see a way to reduce the code duplication needed across all
>> the action modules? I.e. that each of them now needs to register
>> a new per netns subsystem, etc. In other words, is there a way the
>> action API could be reworked to handle most of this in the tc core
>> framework instead?
>
> I definitely agree.
>

Same here.


> Initially I made a wrapper macro for the per netns API for each tc
> action, but it didn't work as I thought, mostly due to the per net ops and
> net_id stuffs.
>
> So it is not as easy as it appears, it needs more work. At least the
> current code is more readable than using any macro. We can always
> refactor the API in the future, and as I mentioned in the changelog that
> is in my plan.
>
> Or do you have any quick and easy way to reduce the code?
>

I did a quick look and i am struggling with it. The patch seems largish
The issue is that we need to do this per kernel module so the code
churn maybe unavoidable; hinfo stored in act_ops complicates things.
Having said that:  All the pernet operations in your
code seem to be generic, other than to accomodate for module specific
act_ops. Is it possible to make generic pernet operations? This way
you could do things at tcf_register_action() for all actions.
The challenge seems to be in the xxx_net_id which appears(sorry didnt
look closely at the namespace code) to need to be unique id per module
and per namespace instance - otherwise i would suggest for xxx_net_id
to be part of act_ops. Could we not have an #ifdef in the namespace
core like the netfilter code does and have one level of indirection
for everything but the namespace 0?

cheers,
jamal

  reply	other threads:[~2016-02-22 13:42 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-20  0:43 [Patch net-next] net_sched: add network namespace support for tc actions Cong Wang
2016-02-20 18:36 ` Daniel Borkmann
2016-02-21  0:46   ` Cong Wang
2016-02-22 13:42     ` Jamal Hadi Salim [this message]
2016-02-22 23:51       ` Cong Wang

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=56CB1053.8020102@mojatatu.com \
    --to=jhs@mojatatu.com \
    --cc=daniel@iogearbox.net \
    --cc=netdev@vger.kernel.org \
    --cc=xiyou.wangcong@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.