NS-3 Click Integration (issue3988043)

[mathieu...@gmail.com]

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.

http://codereview.appspot.com/3988043/

[Ruben Merz]

Hi Mathieu,

Thanks for the review. I was expecting your comment on having a more
generic
Queue access API. Actually, I already started looking into the
netdevice Linux
code and I'm definitely willing to work on it..... In fact, 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.

[Mathieu Lacage]

On Wed, Jan 19, 2011 at 13:21, <ruben...@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...@gmail.com>

[Gustavo Carneiro]

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

[tomh...@gmail.com]

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().

http://codereview.appspot.com/3988043/

[tomh...@gmail.com]

> 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/

[tomh...@gmail.com]

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 added to class Ipv4 instead of this
type of solution. Adding virtual Send() to class Ipv4 would better
support tunneling IP-in-IP devices anyway (which was recently discussed
in a users thread).

http://codereview.appspot.com/3988043/

[Ruben Merz]

Hi Tom, hi all,

> 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().

I have a hard time to follow you here. There are two issues, right?
(1) Flow control and (2) queue information. Based on my current
understanding, for flow control, I think we should follow what
dev_queue_xmit followed by hard_start_xmit do: there are in the Linux
kernel netdevice essentially two code paths (a) there is a queuing
discipline installed, dev_queue_xmit enqueue the frame and then pass
the head of the queue to hard_start_xmit (b) there is no queuing
discipline and hard_start_xmit is directly called. If hard_start_xmit
cannot transmit the frame, it will call netif_stop_queue (it marks the
device as being unable to transmit) and returns NETDEV_TX_BUSY.

Now, correct me if I'm wrong, but in the current ns-3 NetDevice code,
there is not a queue implemented everywhere: CsmaNetDevice has one but
WifiNetDevice has none. Actually, for Wifi, the queue is in the
WifiMac code, i.e. it would correspond to a hardware queue.
CsmaNetDevice corresponds to case (a) above and WifiNetDevice
corresponds to case (b).

I could implement something akin to netif_stop_queue and
netif_wake_queue in ns-3 NetDevice. In addition, there would be a need
for code that checks the underlying MAC layer whether a transmission
is possible or get a notification that transmission is again feasible.
IsTxBusy could remain to give feedback to the upper layer.

For queue information to the upper layer, I don't know what's best.
I'm happy to give a shot at it and discuss it further. But I find it a
pity that it prevents the Click module from being merged.

> http://codereview.appspot.com/3988043/

[Tom Henderson]

On 01/21/2011 12:58 PM, Ruben Merz wrote:
> Hi Tom, hi all,
>
>
>> 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().
>

> I have a hard time to follow you here.There are two issues, right?

> (1) Flow control and (2) queue information. Based on my current
> understanding, for flow control, I think we should follow what
> dev_queue_xmit followed by hard_start_xmit do: there are in the Linux
> kernel netdevice essentially two code paths (a) there is a queuing
> discipline installed, dev_queue_xmit enqueue the frame and then pass
> the head of the queue to hard_start_xmit (b) there is no queuing
> discipline and hard_start_xmit is directly called. If hard_start_xmit
> cannot transmit the frame, it will call netif_stop_queue (it marks the
> device as being unable to transmit) and returns NETDEV_TX_BUSY.

Following this approach sounds good to me.

>
> Now, correct me if I'm wrong, but in the current ns-3 NetDevice code,
> there is not a queue implemented everywhere: CsmaNetDevice has one but
> WifiNetDevice has none. Actually, for Wifi, the queue is in the
> WifiMac code, i.e. it would correspond to a hardware queue.
> CsmaNetDevice corresponds to case (a) above and WifiNetDevice
> corresponds to case (b).
>
> I could implement something akin to netif_stop_queue and
> netif_wake_queue in ns-3 NetDevice. In addition, there would be a need
> for code that checks the underlying MAC layer whether a transmission
> is possible or get a notification that transmission is again feasible.
> IsTxBusy could remain to give feedback to the upper layer.
>
> For queue information to the upper layer, I don't know what's best.
> I'm happy to give a shot at it and discuss it further. But I find it a
> pity that it prevents the Click module from being merged.
>
>> http://codereview.appspot.com/3988043/

To be clear, I do not want to block Click merging on implementing all of
this. In fact, I would prefer to get it merged very soon despite the
build time issues especially since this is GSOC code (same is true for
OpenFlow).

However, since the issue is raised and fresh in people's minds, I would
like to see some discussion and agreement on the bigger picture plan
before merging the small piece needed for Click.

- Tom

[suresh...@gmail.com]

Reviewers: Mathieu Lacage, Ruben Merz, gjcarneiro, Tom Henderson,

Message:
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

Please review this at http://codereview.appspot.com/3988043/

Affected files:
A doc/manual/source/click.rst
M src/devices/wifi/dca-txop.cc
M src/devices/wifi/dca-txop.h
M src/devices/wifi/regular-wifi-mac.cc
M src/devices/wifi/regular-wifi-mac.h
M src/devices/wifi/wifi-mac.h
M src/devices/wifi/wifi-net-device.cc
M src/devices/wifi/wifi-net-device.h
M src/internet-stack/tcp-l4-protocol.cc
M src/internet-stack/udp-l4-protocol.cc
M src/internet-stack/wscript
M src/node/ipv4.h
M src/node/net-device.cc
M src/node/net-device.h
A src/routing/click/doc/click.h
A src/routing/click/examples/nsclick-ip-router.click
A src/routing/click/examples/nsclick-lan-single-interface.click
A src/routing/click/examples/nsclick-raw-wlan.cc
A src/routing/click/examples/nsclick-routing-node0.click
A src/routing/click/examples/nsclick-routing-node2.click
A src/routing/click/examples/nsclick-routing.cc
A src/routing/click/examples/nsclick-simple-lan.cc
A src/routing/click/examples/nsclick-udp-client-server-csma.cc
A src/routing/click/examples/nsclick-udp-client-server-wifi.cc
A src/routing/click/examples/nsclick-wifi-single-interface.click
A src/routing/click/examples/wscript
A src/routing/click/helper/click-internet-stack-helper.cc
A src/routing/click/helper/click-internet-stack-helper.h
A src/routing/click/model/ipv4-click-routing.cc
A src/routing/click/model/ipv4-click-routing.h
A src/routing/click/model/ipv4-external-routing.cc
A src/routing/click/model/ipv4-external-routing.h
A src/routing/click/model/ipv4-l3-click-protocol.cc
A src/routing/click/model/ipv4-l3-click-protocol.h
A src/routing/click/test/ipv4-click-routing-test.cc
A src/routing/click/waf
A src/routing/click/wscript
M src/wscript
M test.py

[Ruben Merz]

Tom, all,

I don't have code yet, but I've spent time going into netdevice.h and
understand what could be implemented in ns-3 NetDevice.

I will start with a short summary of what is relevant in Linux
netdevice.h (based on 2.6.27, see
http://lxr.free-electrons.com/source/include/linux/netdevice.h). I'll
then propose what could be done in ns-3 NetDevice. If no one
complains, I'll do it.

The netdevice entry point is queue_dev_xmit, the output (to the real
driver) is hard_start_xmit. There are between the two, two code
paths. One through queue(s) (the qdisc queue(s)), one directly to
hard_start_xmit. The "queue state(s)" of a netdevice, whether it is
the
qdisc queue(s) or the the underlying tx_ring from the hardware, can be
changed with the function netif_tx_start_queue, netif_tx_stop_queue
and netif_tx_wake_queue. These functions can support one or multiple
queues and set a queue state flag (typically, QUEUE_STATE_XOFF).

The function netif_tx_queue_stopped can then be used to check whether
a particular queue of a netdevice is stopped. Final comment, to
interact with the qdisc queues, you use tc.

Now, from an ns-3 point of view, here is what I'd like to do:

In NetDevice, I'd like to implement an equivalent to the queue state
flag. This should be per queue. Typically

int QueueState (int queueIndex);

This function would be public and it could return the current queue
size and -1 (or some constant) if the queue is full. It is up to the
caller to know which queue to call. Some additional functions like

int GetNQueue(void);

Should be implemented to help the upper layer.

Now, I envision two paths. The simpler one is to do the rest like in
the current patch. Whenever QueueState is called, it checks directly
the necessary queues, either in NetDevice, either in the underlying
MAC. Another path is to follow a bit more the Linux way: i.e.
I could also implement

bool TxStartQueue(int queueIndex);
bool TxStopQueue(int queueIndex);
bool TxQueue(int queueIndex);

These functions would be private, per queue, and would set a flag
equivalent to QUEUE_STATE_XOFF). Now, because we have cases where a
queue is implemented (1) within the NetDevice (the CSMA one) or (2)
outside (the Wifi) case (is there both), there should be callbacks
that allow the underlying layer to notify a queue stop and queue
wake. For instance, if the WifiMacQueue is full, a queue stop should
be notified. If packets are dequeued, a wake queue should be called.

Actually, only the wake queue callback could be
implemented. TxStopQueue could be called directly in NetDevice based
on the return status of mac->Enqueue.

What do you folks think?

Ruben

[Ruben Merz]

[snip]

> Now, I envision two paths. The simpler one is to do the rest like in
> the current patch. Whenever QueueState is called, it checks directly
> the necessary queues, either in NetDevice, either in the underlying
> MAC.

On second thought, the simpler path is a bad idea. Because in the case
the queue(s) is/are not maintained in the NetDevice, that will force
the NetDevice to have knowledge of one or more queues in the lower
layer.

[snip]

[Mathieu Lacage]

[sorry for being so slow to answer: I always forget to read this ML]

On Fri, Jan 28, 2011 at 16:18, Ruben Merz <ruben...@ieee.org> wrote:
> In NetDevice, I'd like to implement an equivalent to the queue state
> flag. This should be per queue. Typically

This is what I had in mind. maybe call it GetQueueState or
GetQueueSize instead :)

> int QueueState (int queueIndex);
>
> This function would be public and it could return the current queue
> size and -1 (or some constant) if the queue is full. It is up to the
> caller to know which queue to call. Some additional functions like
>
> int GetNQueue(void);

yes. Maybe GetNQueues instead.

>
> Should be implemented to help the upper layer.
>
> Now, I envision two paths. The simpler one is to do the rest like in
> the current patch. Whenever QueueState is called, it checks directly
> the necessary queues, either in NetDevice, either in the underlying
> MAC. Another path is to follow a bit more the Linux way: i.e.
> I could also implement
>
> bool TxStartQueue(int queueIndex);
> bool TxStopQueue(int queueIndex);
> bool TxQueue(int queueIndex);
>
> These functions would be private, per queue, and would set a flag
> equivalent to QUEUE_STATE_XOFF). Now, because we have cases where a
> queue is implemented (1) within the NetDevice (the CSMA one) or (2)
> outside (the Wifi) case (is there both), there should be callbacks
> that allow the underlying layer to notify a queue stop and queue
> wake. For instance, if the WifiMacQueue is full, a queue stop should
> be notified. If packets are dequeued, a wake queue should be called.
>
> Actually, only the wake queue callback could be
> implemented. TxStopQueue could be called directly in NetDevice based
> on the return status of mac->Enqueue.

I know that the above mirrors pretty well the linux way of doing things.

Let me try to summarize this from the perspective of what different
users of this class need to do:

higher layers need to:
1) know the current state of the queue ('stopped' or not)
2) know when the state changes.

And the lower layers need to:
3) tell the current state of the queue whenever asked
4) notify whenever the state changes

Is the above description correct ? (I never understood why the higher
layer needed to 'stop' the queues in the Linux kernel which is why
it's not included here)

If this is sufficient, I would suggest instead something like this:

// set a callback invoked whenever a queue changes state. reports queue index.
virtual void SetQueueStateChange (Callback<void,int> callback);
virtual int GetQueueState (int index);
virtual int GetNQueues (void);

What do you think ?

[Gustavo Carneiro]

On Mon, Jan 31, 2011 at 15:26, Mathieu Lacage <mathieu...@gmail.com> wrote:

// set a callback invoked whenever a queue changes state. reports queue index.
virtual void SetQueueStateChange (Callback<void,int> callback);

Ahem... SetQueueStateChangeCallback, if you please.. else looks weird.

 

[Tom Henderson]

It seems that there is lazy consensus that something along the lines
of Ruben's proposal (with Mathieu's comments) is the way to proceed.
How can we push this along?

- is someone going to try to prototype this?
- can this be decoupled from the rest of click so that all of click
support does not block on resolving this queue issue?

[Ruben Merz]

Hi,

On Feb 7, 3:27 pm, Tom Henderson <t...@tomh.org> wrote:
> It seems that there is lazy consensus that something along the lines
> of Ruben's proposal (with Mathieu's comments) is the way to proceed.
> How can we push this along?
>
> - is someone going to try to prototype this?

Yes, I will do it. But I have a paper deadline for this end of the
week. So not this week.

> - can this be decoupled from the rest of click so that all of click
> support does not block on resolving this queue issue?

Yes. I'll coordinate with Suresh to produce a patch that does not
depend on this feature.
Ruben

[Lalith Suresh]

Hello,

On Mon, Feb 7, 2011 at 3:27 PM, Tom Henderson <to...@tomh.org> wrote:

- can this be decoupled from the rest of click so that all of click
support does not block on resolving this queue issue? 

I've uploaded a new patch set on the code review issue for ns-3-click without tx-queue feedback.

 

--
You received this message because you are subscribed to the Google Groups "ns-3-reviews" group.
To post to this group, send email to ns-3-r...@googlegroups.com.
To unsubscribe from this group, send email to ns-3-reviews...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/ns-3-reviews?hl=en.

--
Lalith Suresh

Department of Computer Science and Engineering

Instituto Superior Técnico
www.lalith.in