I did not review the click-specific code but I looked at the NetDevice changes and ...
14 years, 3 months ago
(2011-01-19 12:07:44 UTC)
#1
I did not review the click-specific code but I looked at the NetDevice changes
and I tried to figure out how they fit in the global picture. My main comment is
that I don't like the minimal approach chosen to export queue information at the
NetDevice API level. i.e., I would prefer a proposal that attempts to provide
full access to the underlying queue(s) capabilities and that proposal should not
export the ns3::Queue type to allow NetDevice implementations to not necessarily
use the ns3::Queue type if they don't want to.
http://codereview.appspot.com/3988043/diff/2002/src/node/net-device.h
File src/node/net-device.h (right):
http://codereview.appspot.com/3988043/diff/2002/src/node/net-device.h#newcode218
src/node/net-device.h:218: virtual bool IsTxBusy (void);
The method name seems to be a bit wrong: this is really a method to know whether
or not the tx queue is full so, I would name it IsTxQueueFull.
However, long term, it is pretty clear that we should have a way to expose the
underlying tx queues in the device API because a lot of higher layers need this
kind of information. Typical things that you need:
1) know how many tx queues are present in the underlying device
2) know if you can queue a packet of a specific size in a specific queue
3) when the queue will be able to accept a packet of a specific size in the
future
There is quite a lot to be learned from recent versions of the net_device API in
the linux kernel so, any NetDevice API that attempts to extend knowledge about
queues and queue state should try to see how we can deal with the above issues
which I believe are already addressed in the linux kernel.
Hi Mathieu, Thanks for the review. I was expecting your comment on having a more ...
14 years, 3 months ago
(2011-01-19 12:21:13 UTC)
#2
Hi Mathieu,
Thanks for the review. I was expecting your comment on having a more generic
Queue name API. Actually, I already started looking into the netdevice Linux
code and I'm definitely willing to work on it..... Actually, the name IsTxBusy
comes from there: as far as I understand right now, the netdevice does not
export queue information but simply returns or give access to information on its
current transmission status. TxBusy is typically set when the netdevice cannot
accept any more traffic.
That said, could we separate this issue from the Click code here? Would you be
willing to accept the temporary solution currently in the patchset? I can't
right now put a lot of time on doing it.
On Wed, Jan 19, 2011 at 13:21, <ruben.merz@telekom.de> wrote: > Hi Mathieu, > > Thanks ...
14 years, 3 months ago
(2011-01-20 11:58:09 UTC)
#3
On Wed, Jan 19, 2011 at 13:21, <ruben.merz@telekom.de> wrote:
> Hi Mathieu,
>
> Thanks for the review. I was expecting your comment on having a more
> generic Queue name API. Actually, I already started looking into the
> netdevice Linux code and I'm definitely willing to work on it.....
> Actually, the name IsTxBusy comes from there: as far as I understand
> right now, the netdevice does not export queue information but simply
> returns or give access to information on its current transmission
> status. TxBusy is typically set when the netdevice cannot accept any
> more traffic.
Yes, this is the linux kernel vocabulary/model. For the sake of
alignment, your proposal makes sense but the linux name still sucks.
> That said, could we separate this issue from the Click code here? Would
Knowing tom, he would probably say yes.
I would say no because I know I can exploit you to fix the bigger
problem because you have an incentive to. If I remove the incentive
(allow you to merge as-is), the probability that someone will fix this
decreases.
> you be willing to accept the temporary solution currently in the
> patchset? I can't right now put a lot of time on doing it.
I have said this a couple of times privately to tom and I think I said
this publicly on the MLs also but I feel that right now, we should
invest all our resources on fixing our build to be modular rather than
merge new code which is going to increase the pressure on our
monolithic build system. So, if you can't do it now, I would say, it's
even better because it means we have one less module to merge.
I am evil :/
I wish I had time to help deal with these issues myself but it is not
the case right now so I don't feel legitimate to block this because I
can't offer a constructive option but I think that my above proposal
makes sense and should be followed.
Mathieu
--
Mathieu Lacage <mathieu.lacage@gmail.com>
On Thu, Jan 20, 2011 at 11:58, Mathieu Lacage <mathieu.lacage@gmail.com>wrote: > On Wed, Jan 19, ...
14 years, 3 months ago
(2011-01-20 12:05:22 UTC)
#4
On Thu, Jan 20, 2011 at 11:58, Mathieu Lacage <mathieu.lacage@gmail.com>wrote:
> On Wed, Jan 19, 2011 at 13:21, <ruben.merz@telekom.de> wrote:
> > Hi Mathieu,
> >
> > Thanks for the review. I was expecting your comment on having a more
> > generic Queue name API. Actually, I already started looking into the
> > netdevice Linux code and I'm definitely willing to work on it.....
> > Actually, the name IsTxBusy comes from there: as far as I understand
> > right now, the netdevice does not export queue information but simply
> > returns or give access to information on its current transmission
> > status. TxBusy is typically set when the netdevice cannot accept any
> > more traffic.
>
> Yes, this is the linux kernel vocabulary/model. For the sake of
> alignment, your proposal makes sense but the linux name still sucks.
>
> > That said, could we separate this issue from the Click code here? Would
>
> Knowing tom, he would probably say yes.
>
> I would say no because I know I can exploit you to fix the bigger
> problem because you have an incentive to. If I remove the incentive
> (allow you to merge as-is), the probability that someone will fix this
> decreases.
>
>
> > you be willing to accept the temporary solution currently in the
> > patchset? I can't right now put a lot of time on doing it.
>
> I have said this a couple of times privately to tom and I think I said
> this publicly on the MLs also but I feel that right now, we should
> invest all our resources on fixing our build to be modular rather than
> merge new code which is going to increase the pressure on our
> monolithic build system. So, if you can't do it now, I would say, it's
> even better because it means we have one less module to merge.
>
I am not saying I disagree with you, but I think for GSOC modules it makes
sense to open an exception and merge them anyway. I mean, since Google
kindly funded development of ns-3 code, it's a shame to risk letting the
code bitrot before being included in main ns-3.
Best regards,
--
Gustavo J. A. M. Carneiro
INESC Porto, UTM, WiN, http://win.inescporto.pt/gjc
"The universe is always one step beyond logic." -- Frank Herbert
On 2011/01/19 12:07:44, Mathieu Lacage wrote: > I did not review the click-specific code but ...
14 years, 3 months ago
(2011-01-21 20:03:06 UTC)
#5
On 2011/01/19 12:07:44, Mathieu Lacage wrote:
> I did not review the click-specific code but I looked at the NetDevice changes
> and I tried to figure out how they fit in the global picture. My main comment
is
> that I don't like the minimal approach chosen to export queue information at
the
> NetDevice API level. i.e., I would prefer a proposal that attempts to provide
> full access to the underlying queue(s) capabilities and that proposal should
not
> export the ns3::Queue type to allow NetDevice implementations to not
necessarily
> use the ns3::Queue type if they don't want to.
>
What would you prefer to export here? I know in the past you avoided using
Queue for Wifi so I'm wondering what common interface you would like to see and
how to support it.
One possibility might be to store queues as objects and require them to
aggregate an interface based on struct netdev_queue. We would need also to
extend the netdevice API to export number of (tx) queues.
For backpressure, something like the proposed IsTxBusy() would be OK with me or
else generalize it to a TxStatus() method to return an enum based on linux
netdev_tx. Or, make NetDevice::Send return this enum instead of bool.
Next, net_device_ops.ndo_select_queue() might be supported, and add a packet tag
to be analogous to skb_set_queue_mapping().
> Knowing tom, he would probably say yes. > I would usually say yes but ...
14 years, 3 months ago
(2011-01-21 20:04:21 UTC)
#6
> Knowing tom, he would probably say yes.
>
I would usually say yes but in this case, I would prefer to try to sort out the
long term proposal while the issue is fresh.
http://codereview.appspot.com/3988043/diff/2002/src/internet-stack/tcp-l4-protocol.cc File src/internet-stack/tcp-l4-protocol.cc (right): http://codereview.appspot.com/3988043/diff/2002/src/internet-stack/tcp-l4-protocol.cc#newcode130 src/internet-stack/tcp-l4-protocol.cc:130: if (!isClickNode) I would prefer to see Send() API ...
14 years, 3 months ago
(2011-01-21 20:07:00 UTC)
#7
Hello all, I've incorporated Tom's comment which adds a virtual Send() method to the Ipv4 ...
14 years, 3 months ago
(2011-01-22 13:55:35 UTC)
#8
Hello all,
I've incorporated Tom's comment which adds a virtual Send() method to the Ipv4
class, and makes the necessary corrections in Tcp/UdpL4protocol.
--
Lalith
josh insisted that I review this so, here it is. http://codereview.appspot.com/3988043/diff/29001/src/internet-stack/tcp-l4-protocol.cc File src/internet-stack/tcp-l4-protocol.cc (right): http://codereview.appspot.com/3988043/diff/29001/src/internet-stack/tcp-l4-protocol.cc#newcode369 ...
14 years, 2 months ago
(2011-02-17 14:04:43 UTC)
#9
a few more comments on the latest patchset http://codereview.appspot.com/3988043/diff/35001/src/node/ipv4.h File src/node/ipv4.h (right): http://codereview.appspot.com/3988043/diff/35001/src/node/ipv4.h#newcode147 src/node/ipv4.h:147: * ...
14 years, 2 months ago
(2011-02-18 02:06:37 UTC)
#11
Thanks Tom. Mathieu? :) On Fri, Feb 18, 2011 at 7:33 PM, <tomh.org@gmail.com> wrote: > ...
14 years, 2 months ago
(2011-02-19 09:13:21 UTC)
#15
Thanks Tom.
Mathieu? :)
On Fri, Feb 18, 2011 at 7:33 PM, <tomh.org@gmail.com> wrote:
> I don't have further comments and support merging it for this release
> once you believe that all items of concern have been addressed.
>
>
>
>
> http://codereview.appspot.com/3988043/
>
--
Lalith Suresh
*Department of Computer Science and Engineering*
*Instituto Superior Técnico*
www.lalith.in
Issue 3988043: NS-3 Click Integration
(Closed)
Created 14 years, 3 months ago by Lalith Suresh
Modified 14 years ago
Reviewers: Mathieu Lacage, Ruben Merz, gjcarneiro, Tom Henderson
Base URL:
Comments: 29
