Changed and refactored the code #2

freysteinn · 2021-06-03T17:57:50Z

Hi all.

After some reflection from our last meeting and working on the code, I decided to change and refactor the code. I split the data structures and the scheduling algorithms to make it more transparent which part is which. I also removed the packet awareness from the queues to make them able to have references to packets, flows, or queues without needing helpers.

This pull request breaks the Eiffel files for now. I am currently working on fixing them using the new changes. One of the things that I am fixing now is to make a better approach to handle flow-ids. When I wanted to implement the HPFQ scheduler from the PIFO paper, the queuing algorithms always looks up the flow-ids from within the packet. This becomes an issue when I want to schedule a reference to a queue instead of a packet. Therefore, I am thinking about changing the code to pass the flow-id as a parameter to the enqueue function. It will still use the flow-id from the packet as an initial reference. However, when we nest schedulers, we can decouple the flow-id from the packet and pass it with the queue reference. I think this will make the code more flexible and make it easier to implement more complex schedulers. However, it does look a little uglier for the more straightforward cases.

Edit: I changed the interface to now include references with the packets. This change makes it possible to queue a reference but still rank the packet or flow from the flow-id without detaching it.

Best wishes
Frey Alfredsson

I added shebang to all runnable Python programs. I also added keyword arguments to the packet definitions to make it more straightforward for the reader which parameters are flow, ids, and length. I found it a bit hard to distinguish at first glance. Signed-off-by: Frey Alfredsson <[email protected]>

I decided to split the pifo-basic.py file into two separate packet scheduling files. The program prints the added packets which are already in FIFO order. Therefore, I don't see a point in including the FIFO output as well in the examples. Signed-off-by: Frey Alfredsson <[email protected]>

I changed and refactored the framework; for now, it breaks the current scheduling algorithms. I have provided two new example schedulers, FIFO and SRPT, to show how the new framework works. I split the data structures and the scheduling algorithms to make it more transparent which part is which. I also removed the packet awareness from the queues to make them able to have references to packets, flows, or queues without needing helpers. This change makes it easier to create hierarchical schedulers and to track flows in schedulers like SRPT. Signed-off-by: Frey Alfredsson <[email protected]>

This commit adds the following two scheduling algorithms to the repo using the new framework: - Weighted Fair Queueing (WFQ) - Start-Time Fair Queuing (STFQ) Signed-off-by: Frey Alfredsson <[email protected]>

Signed-off-by: Frey Alfredsson <[email protected]>

This commit adds the HierarchicalPacket Fair Queueing (HPFQ) scheduler from the paper "Programmable packet scheduling at line rate" by Sivaraman, Anirudh, et al. This commit also changes the framework to make it easy to schedule references to flows, queues, or schedulers. It does so by adding a reference with every enqueue operation. By adding the reference with the packet or flow, we can create a rank from packet or flow and queue the reference. This commit adds the HierarchicalPacket Fair Queueing (HPFQ) scheduler from the paper "Programmable packet scheduling at line rate" by Sivaraman, Anirudh, et al. This commit also changes the framework to make it easy to schedule references to flows, queues, or schedulres. Signed-off-by: Frey Alfredsson <[email protected]>

tohojo

OK with the refactor, but especially the HPFQ implementation has a few issues. I tried to do my best with the Github comment interface, but let me know if something is not clear in the comments.

Also, as general point, writing about yourself in first person in the commit message is a bit odd. Better to talk about the code; what does it do, and most importantly why. You do that in later commits, so that's good. The HPFQ commit message is dupliated, tough... and what's the slides doing in the middle of this PR? :)

tohojo · 2021-06-07T11:03:13Z

queue-exp/pifo_lib.py

-    def __init__(self, pkts, queue):
+    """This class is responsible for running a test on a packet scheduling
+    algorithm. It is accountable for enquing and dequeing packets. For now, it
+    does so by dequing as many packets as it enqued. In the next iteration, when


Needs more vowels! :) ('dequeueing' and 'enqueued')

See https://en.wikipedia.org/wiki/Queueing_theory#Spelling

Ha ha, yes. It also just feels uncomfortable to write with too many vowels as well :D

tohojo · 2021-06-07T11:04:25Z

queue-exp/pifo_srpt.py

+
+        # We cheat by accessing the global packet list directly
+        for pkt in pkts:
+            if pkt.flow in self._remains.keys():


the 'in' operator works directly on dictionaries, no need to use .keys()

Ah, of course. I knew that. I was overthinking that I need to do this when working with values() and forgot myself.

tohojo · 2021-06-07T11:04:58Z

queue-exp/pifo_lib.py

-        return itm.length if itm else 0
+        result = 0
+        for itm in self._list:
+            result += itm.length if itm else 0


That would be too obvious, right? Ha ha

tohojo · 2021-06-07T11:06:07Z

queue-exp/pifo_srpt.py

+        self._remains = {}
+
+        # We cheat by accessing the global packet list directly
+        for pkt in pkts:


where is pkts coming from? this looks like it'll crash?

I thought of this as the typical example given in operating systems courses where you are supposed to write a nonexistent and optimal scheduling algorithm. I was not too fond of these examples. After all, it is impossible to implement because it needs complete knowledge of each process's remaining time beforehand. Is it different here? Would we reschedule depending on the currently known flow size instead?

Ah, now I see what you're trying to do. I think the idea for an actual SRPT algorithm is that the sender will tag each packet with its remaining size in a custom header; that's how they explain it in the Pfabric paper (see the start of §4). This also fits with the "implementation" in the Pifo code, where it just extracts a ranking from the packet...

tohojo · 2021-06-07T11:08:03Z

queue-exp/pifo_srpt.py

+
+    def get_rank(self, pkt):
+        rank = self._remains[pkt.flow]
+        self._remains[pkt.flow] -= pkt.length


Why is a get_rank() function modifying the state? That seems like a surprising side effect.
Also, why is it subtracting? :)

I modeled the get_rank function of this algorithm after the Stfq. It is supposed to rank the packets on their remaining flow length given absolute knowledge of the flows. In Stfq, we similarly track the information and update the tracking information in the get_rank function, except we track it based on how many packets we have seen of the flow so far.

Here is how the PIFO paper's companion repo refers to the ranking of the packets.
https://github.com/programmable-scheduling/pifo-machine/blob/master/examples/srpt.dot

Yeah, right, I did realise later that this was similar to the other get_rank() functions; just forgot to go back and edit this comment; sorry about that!

And now that I grok'ed what you are tying to do with the above "initialise from global state" thing I also see why it's decrementing. However, if you're going to have an implementation of SRPT here I think it makes more sense to just assume the length is in the packet. Kinda makes the algorithm itself trivial, but what can you do? :)

tohojo · 2021-06-07T11:30:40Z

queue-exp/pifo_wfq.py

+
+
+class Wfq(SchedulingAlgorithm):
+    """Weighted Fair Queueing (WFQ)"""


So "WFQ" is really "STFQ + weights", right? So why not just make this a subclass of Stfq (or just straight-up add the weights as an optional feature to the Stfq class itself)? Also, the commit message that adds this is a bit misleading: it says it's "adding" stfq, when it's really just changing the implementation to use the SchedulingAlgorithm class; so better make that a separate commit, then add the weights as a separate thing. (The github review interface is terrible in that it won't let me comment on the commit message, so just sticking that in here).

Yes, I agree. It's terrible not to be able to comment on specific commits. Maybe I should also try to make more PR instead to make it more clear.

I agree it's not that much of a change to warrant the new name. I did this because the PIFO paper and examples talk about them separately, even though they look pretty much the same.

I think it's OK to keep them separate if you think that makes things clearer; but then make the relationship between them explicit; i.e., add an update_last_finish(flow, rank, pkt) method to stfq and make WFQ be a subclass of STFQ that only overrides that one method...

tohojo · 2021-06-07T11:31:54Z

queue-exp/pifo_srpt.py



 class Srpt(SchedulingAlgorithm):
    """Shortest Remaining Processing Time"""

-    def __init__(self):
+    def __init__(self, name=None):


This 'name' stuff looks like a separate refactoring, why is it in the 'HPFQ' commit?

Yes, you are right. I did add it after. However, I decided to keep it with the HPFQ commit because that is what drove that change. When you run the HPFQ example, it was hard to see which reference referred to the left and right WFQ in the root, so I added the names to the algorithms to distinguish them. It would have been easier to follow in a separate commit. I will try to keep that in mind in the future when I make changes like this.

Totally get why you needed it! However, when reviewing, such "unrelated" changes make it harder to pick out the important bits, so it's better to split them into a separate commit and just point out that they will be used by the subsequent commit... So yeah, please split them in the future :)

tohojo · 2021-06-07T11:42:45Z

queue-exp/sched_lib.py

@@ -48,7 +48,7 @@ def run(self):
        print("  Inserting packets into scheduler:")
        pprint(self.input_pkts, indent=4)
        for p in self.input_pkts:
-            self.scheduler.enqueue(p)
+            self.scheduler.enqueue(p, p)


When you get a call like this it's a sign that the API is bad. The 'enqueue' function takes a single packet to be enqueued, don't make it jump through hoops because of bad abstractions...

I really wanted to avoid making it look like that. The problem was when enquing references to queues or scheduling algorithms like I did in HPFQ. Then the queue did not have ranking information. I wanted the ranking function to rank either the passed packet or flow and then queue the reference. In the HPFQ, the root WFQ couldn't rank a reference to a queue, but it could rank a packet or a flow. That's why I decided to separate these two parameters as a reference and the packet or flow. However, now when I think about it, adding the reference as a field to the packet or flow would make more sense. Then the PIFO could check if the reference is not "None" and queue that instead of the packet. Then we would only have one parameter, and algorithms that rely on the reference can track them. For instance, we could use this reference field with the pacer to track which queue the scheduler needs to add the packet to next.

By the way, do we have a word for something that could be either a packet or a flow? I find calling the parameter "item" to be rather ugly. Worst case, maybe pktflw? I still find it ugly.

I think that could work. And no, 'item' was the best I could come up with. 'pktflw' is worse, IMO...

tohojo · 2021-06-07T11:44:06Z

queue-exp/pifo_hpfq.py

+            queue = self._left
+        else:
+            self._right.enqueue(ref, item)
+            queue = self._right


Okay, so the logical flow here is "pick a queue from left or right, and enqueue the packet into it". So it doesn't need to arguments to enqueue here either, just pass through the 'item'...

Yes, I am going to change this by adding the reference to the item (packet or flow) instead. Then this won't look like this.

tohojo · 2021-06-07T12:04:26Z

queue-exp/pifo_hpfq.py

+            self._right.enqueue(ref, item)
+            queue = self._right
+
+        self._root.enqueue(queue, item)


And here you're also only enqueueing a single ref to the child queue. You do need the packet to get the length, but the algorithm ends up being wrong: in the root WFQ you'll end up indexing the packet completions by the actual packet flow, not the child queue. So it ends up sorting packets by flows just like the leaf nodes do, meaning it won't actually be hierarchical at all...

I think this weirdness comes from mixing of abstraction levels: you've implemented WFQ/STFQ as SchedulingAlgorithms, which has a clear "enqueue this packet" API (i.e., one arg to enqueue()). But here you're implementing a new SchedulingAlgorithm that reuses the others as sub-primitives. Where really, the primitive is a PIFO, all the different SchedulingAlgorithms just happen to reuse the same logic for get_rank()

One way around this would be to specify a common base class for all three algorithms instead, which implements a get_rank that does the last_finish tracking, but takes the 'flow' as parameter. And then STFQ/WFQ could pass in an actual flow there, and HPFQ could pass in the child queues.

Yes, that's correct. That's an error on my part. It makes much more sense to schedule the length of the queue to make it hierarchical.

I think we aim to do two things. One is to understand the algorithms and agree on them, and the other is to find an interface that helps us write the eBPF abstraction of the code. I think we should have three different PIFOs inside the SchedulingAlgorithm and not try to reuse other SchedulingAlgorithm classes directly. That way, we are getting a little closer to what we are thinking with the eBPF code without introducing too many Python-specific abstractions. If we add more inheritance to the code, I think it makes sense from a Python and objected-oriented approach. However, I believe the SchedulingAlgorithm class should be closer to what we want the eBPF C code to look like, even if it is a bit Pythonized.

freysteinn · 2021-06-07T17:42:43Z

OK with the refactor, but especially the HPFQ implementation has a few issues. I tried to do my best with the Github comment interface, but let me know if something is not clear in the comments.

Excellent, I will try my best to explain which parts I find hard to understand. I find the interface a bit strange when I write a direct reply to the first comment, so this reply comes after I have already replied to a few of your comments.

Also, as general point, writing about yourself in first person in the commit message is a bit odd. Better to talk about the code; what does it do, and most importantly why. You do that in later commits, so that's good. The HPFQ commit message is dupliated, tough... and what's the slides doing in the middle of this PR? :)

Yes, when I wrote that, I was more writing this like the technical report. I will talk about the code in the future. :)

That's my fault. I tend to copy the comments into a grammar checker and then back into Emacs; then, I forgot to remove it. I will fix this and try not to do it again. I also accidentally pushed the meetings into the commit when it was supposed to go directly into the main repository. I need to relax more before I push and read my commits thoroughly. Obviously, I know these sloppy mistakes would not fly in other repositories, so it should not apply here either. :)

freysteinn added 4 commits June 3, 2021 20:08

Added scheduling algorithms STFQ and WFQ

0f1602f

This commit adds the following two scheduling algorithms to the repo using the new framework: - Weighted Fair Queueing (WFQ) - Start-Time Fair Queuing (STFQ) Signed-off-by: Frey Alfredsson <[email protected]>

freysteinn force-pushed the master branch from cd0be7d to 0f1602f Compare June 3, 2021 18:08

Added missing older meeting slides

145b33d

Signed-off-by: Frey Alfredsson <[email protected]>

freysteinn requested a review from tohojo June 6, 2021 20:55

freysteinn force-pushed the master branch from 2c5b107 to 1d341c1 Compare June 6, 2021 21:50

tohojo requested changes Jun 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed and refactored the code #2

Changed and refactored the code #2

freysteinn commented Jun 3, 2021 •

edited

Loading

tohojo left a comment

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

freysteinn Jun 7, 2021

tohojo Jun 7, 2021

freysteinn commented Jun 7, 2021



		class Wfq(SchedulingAlgorithm):
		"""Weighted Fair Queueing (WFQ)"""

Changed and refactored the code #2

Are you sure you want to change the base?

Changed and refactored the code #2

Conversation

freysteinn commented Jun 3, 2021 • edited Loading

tohojo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freysteinn commented Jun 7, 2021

freysteinn commented Jun 3, 2021 •

edited

Loading