| Message ID | 1506429324-10416-1-git-send-email-antonio.fischetti@intel.com |
|---|---|
| Headers | show |
| Series | Conntrack: add commands to r/w CT parameters. | expand |
On 09/26/2017 01:35 PM, antonio.fischetti@intel.com wrote: > This series adds two new commands to allow read/write of > some of the CT configuration parameters. This could be > used for maintenance purposes or to find a better tuning > of the current setup. > Hi Antonio. I don't think that helps people not too familiar with conntrack understand why the commands are needed and what cases they will help with. Also, I think there should be some documentation to guide the user on when to use the new commands. I'm not making comment on the usefulness or not of the commands but there's a need to explain why you are making the changes and guide the user on them. thanks, Kevin. > V2: Reworked based on comments. > V1: First implementation. > > Fischetti, Antonio (4): > dpctl: Add a comment to functions retrieving the datapath name. > conntrack: add commands to r/w CT parameters. > conntrack: r/w upper limit connection value. > conntrack: read current nr of connections. > > lib/conntrack.c | 90 +++++++++++++++++++++++++++++++++++++++++++++ > lib/conntrack.h | 3 ++ > lib/ct-dpif.c | 28 ++++++++++++++ > lib/ct-dpif.h | 2 + > lib/dpctl.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++- > lib/dpif-netdev.c | 19 ++++++++++ > lib/dpif-netlink.c | 2 + > lib/dpif-provider.h | 4 ++ > 8 files changed, 251 insertions(+), 1 deletion(-) >
Thanks Kevin, comments inline. -Antonio > -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Monday, October 2, 2017 11:46 AM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2 0/4] Conntrack: add commands to r/w CT > parameters. > > On 09/26/2017 01:35 PM, antonio.fischetti@intel.com wrote: > > This series adds two new commands to allow read/write of > > some of the CT configuration parameters. This could be > > used for maintenance purposes or to find a better tuning > > of the current setup. > > > > Hi Antonio. I don't think that helps people not too familiar with > conntrack understand why the commands are needed and what cases they > will help with. [Antonio] I can rephrase it like: This change comes from the consideration that when the CT is enabled the overall performance can be deeply affected, even with simple firewall rules and with stateless protocols like UDP. This implementation adds a basic infrastructure that allows the user to adjust the CT configuration parameters at run-time in order to find a better tuning. For example - depending on the traffic profile - the user could decrease at run-time the maximum number of tracked connections, so to mitigate the impact on performance. > Also, I think there should be some documentation to > guide the user on when to use the new commands. [Antonio] Sure, I'll update the dpctl.man and possibly other docs too, like some new doc inside Documentation/howto/ ? If you think other docs should be updated/added please let me know. > I'm not making comment > on the usefulness or not of the commands but there's a need to explain > why you are making the changes and guide the user on them. > > thanks, > Kevin. > > > V2: Reworked based on comments. > > V1: First implementation. > > > > Fischetti, Antonio (4): > > dpctl: Add a comment to functions retrieving the datapath name. > > conntrack: add commands to r/w CT parameters. > > conntrack: r/w upper limit connection value. > > conntrack: read current nr of connections. > > > > lib/conntrack.c | 90 +++++++++++++++++++++++++++++++++++++++++++++ > > lib/conntrack.h | 3 ++ > > lib/ct-dpif.c | 28 ++++++++++++++ > > lib/ct-dpif.h | 2 + > > lib/dpctl.c | 104 > +++++++++++++++++++++++++++++++++++++++++++++++++++- > > lib/dpif-netdev.c | 19 ++++++++++ > > lib/dpif-netlink.c | 2 + > > lib/dpif-provider.h | 4 ++ > > 8 files changed, 251 insertions(+), 1 deletion(-) > >
On 10/03/2017 10:11 AM, Fischetti, Antonio wrote: > Thanks Kevin, comments inline. > > -Antonio > >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Monday, October 2, 2017 11:46 AM >> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH v2 0/4] Conntrack: add commands to r/w CT >> parameters. >> >> On 09/26/2017 01:35 PM, antonio.fischetti@intel.com wrote: >>> This series adds two new commands to allow read/write of >>> some of the CT configuration parameters. This could be >>> used for maintenance purposes or to find a better tuning >>> of the current setup. >>> >> >> Hi Antonio. I don't think that helps people not too familiar with >> conntrack understand why the commands are needed and what cases they >> will help with. > > [Antonio] > I can rephrase it like: > This change comes from the consideration that when the CT is enabled > the overall performance can be deeply affected, even with simple > firewall rules and with stateless protocols like UDP. > This implementation adds a basic infrastructure that allows the user > to adjust the CT configuration parameters at run-time in order to > find a better tuning. > For example - depending on the traffic profile - the user could decrease > at run-time the maximum number of tracked connections, so to mitigate > the impact on performance. > Sounds much better, thanks. > >> Also, I think there should be some documentation to >> guide the user on when to use the new commands. > > [Antonio] > Sure, I'll update the dpctl.man and possibly other docs too, like some > new doc inside Documentation/howto/ ? > If you think other docs should be updated/added please let me know. > You could add to the 'performance tuning' section if it's just about getting better performance. I don't really mind where, just that user has enough info to know what they are and why they would use them. thanks, Kevin. >> I'm not making comment >> on the usefulness or not of the commands but there's a need to explain >> why you are making the changes and guide the user on them. >> >> thanks, >> Kevin. >> >>> V2: Reworked based on comments. >>> V1: First implementation. >>> >>> Fischetti, Antonio (4): >>> dpctl: Add a comment to functions retrieving the datapath name. >>> conntrack: add commands to r/w CT parameters. >>> conntrack: r/w upper limit connection value. >>> conntrack: read current nr of connections. >>> >>> lib/conntrack.c | 90 +++++++++++++++++++++++++++++++++++++++++++++ >>> lib/conntrack.h | 3 ++ >>> lib/ct-dpif.c | 28 ++++++++++++++ >>> lib/ct-dpif.h | 2 + >>> lib/dpctl.c | 104 >> +++++++++++++++++++++++++++++++++++++++++++++++++++- >>> lib/dpif-netdev.c | 19 ++++++++++ >>> lib/dpif-netlink.c | 2 + >>> lib/dpif-provider.h | 4 ++ >>> 8 files changed, 251 insertions(+), 1 deletion(-) >>> >
Thanks Kevin, I'll rework a v3. Antonio > -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Tuesday, October 3, 2017 11:11 AM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2 0/4] Conntrack: add commands to r/w CT > parameters. > > On 10/03/2017 10:11 AM, Fischetti, Antonio wrote: > > Thanks Kevin, comments inline. > > > > -Antonio > > > >> -----Original Message----- > >> From: Kevin Traynor [mailto:ktraynor@redhat.com] > >> Sent: Monday, October 2, 2017 11:46 AM > >> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > >> Subject: Re: [ovs-dev] [PATCH v2 0/4] Conntrack: add commands to r/w CT > >> parameters. > >> > >> On 09/26/2017 01:35 PM, antonio.fischetti@intel.com wrote: > >>> This series adds two new commands to allow read/write of > >>> some of the CT configuration parameters. This could be > >>> used for maintenance purposes or to find a better tuning > >>> of the current setup. > >>> > >> > >> Hi Antonio. I don't think that helps people not too familiar with > >> conntrack understand why the commands are needed and what cases they > >> will help with. > > > > [Antonio] > > I can rephrase it like: > > This change comes from the consideration that when the CT is enabled > > the overall performance can be deeply affected, even with simple > > firewall rules and with stateless protocols like UDP. > > This implementation adds a basic infrastructure that allows the user > > to adjust the CT configuration parameters at run-time in order to > > find a better tuning. > > For example - depending on the traffic profile - the user could decrease > > at run-time the maximum number of tracked connections, so to mitigate > > the impact on performance. > > > > Sounds much better, thanks. > > > > >> Also, I think there should be some documentation to > >> guide the user on when to use the new commands. > > > > [Antonio] > > Sure, I'll update the dpctl.man and possibly other docs too, like some > > new doc inside Documentation/howto/ ? > > If you think other docs should be updated/added please let me know. > > > > You could add to the 'performance tuning' section if it's just about > getting better performance. I don't really mind where, just that user > has enough info to know what they are and why they would use them. > > thanks, > Kevin. > > >> I'm not making comment > >> on the usefulness or not of the commands but there's a need to explain > >> why you are making the changes and guide the user on them. > >> > >> thanks, > >> Kevin. > >> > >>> V2: Reworked based on comments. > >>> V1: First implementation. > >>> > >>> Fischetti, Antonio (4): > >>> dpctl: Add a comment to functions retrieving the datapath name. > >>> conntrack: add commands to r/w CT parameters. > >>> conntrack: r/w upper limit connection value. > >>> conntrack: read current nr of connections. > >>> > >>> lib/conntrack.c | 90 +++++++++++++++++++++++++++++++++++++++++++++ > >>> lib/conntrack.h | 3 ++ > >>> lib/ct-dpif.c | 28 ++++++++++++++ > >>> lib/ct-dpif.h | 2 + > >>> lib/dpctl.c | 104 > >> +++++++++++++++++++++++++++++++++++++++++++++++++++- > >>> lib/dpif-netdev.c | 19 ++++++++++ > >>> lib/dpif-netlink.c | 2 + > >>> lib/dpif-provider.h | 4 ++ > >>> 8 files changed, 251 insertions(+), 1 deletion(-) > >>> > >