Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed and refactored the code #2

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions queue-exp/pifo_fifo.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,24 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
"""

from pifo_lib import Packet, Runner, Pifo
from pifo_lib import SchedulingAlgorithm
from sched_lib import Packet, Runner, Pifo, SchedulingAlgorithm


class Fifo(SchedulingAlgorithm):
"""First in, first out (FIFO)"""

def __init__(self):
def __init__(self, name=None):
super().__init__(name)
self._pifo = Pifo()

def enqueue(self, item):
rank = self.get_rank(item)
self._pifo.enqueue(item, rank)

def get_rank(self, item):
def get_rank(self, _):
"""Rank the items in FIFO order."""
return self._pifo.qlen

def enqueue(self, ref, item):
rank = self.get_rank(item)
self._pifo.enqueue(ref, rank)

def dequeue(self):
return self._pifo.dequeue()

Expand Down
87 changes: 87 additions & 0 deletions queue-exp/pifo_hpfq.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#!/usr/bin/env python3
# coding: utf-8 -*-
#
# SPDX-License-Identifier: GPL-3.0-or-later
#
# pifo-hpfq.py

"""HierarchicalPacket Fair Queueing (HPFQ)
This scheduling algorithm is mentioned in the paper "Programmable packet
scheduling at line rate" by Sivaraman, Anirudh, et al. It creates a hierarchy of
WFQ schedulers. The central scheduler is called root and contains references to
other WFQ schedulers. Those two WFQ schedulers are called left and right. We
chose that packets with flow ids lower than ten go into the left scheduler in
our implementation. In contrast, the others go into the right scheduler."""

__copyright__ = """
Copyright (c) 2021, Toke Høiland-Jørgensen <[email protected]>
Copyright (c) 2021, Frey Alfredsson <[email protected]>
"""

__license__ = """
This program is free software: you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation, either version 3 of the License, or
(at your option) any later version.
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>.
"""

from sched_lib import Packet, Runner, SchedulingAlgorithm
from pifo_wfq import Wfq


class Hpfq(SchedulingAlgorithm):
"""HierarchicalPacket Fair Queueing (HPFQ)"""

def __init__(self, name=None):
super().__init__(name)
self._root = Wfq("root")
self._left = Wfq("Left")
self._right = Wfq("Right")

def enqueue(self, ref, item):
queue = None
if item.flow < 10:
self._left.enqueue(ref, item)
queue = self._left
else:
self._right.enqueue(ref, item)
queue = self._right
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


self._root.enqueue(queue, item)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, SGTM.


def dequeue(self):
queue = self._root.dequeue()
return queue.dequeue() if queue is not None else None

def dump(self):
print(" Root:")
self._root.dump()
print(" Left:")
self._left.dump()
print(" Right:")
self._right.dump()


if __name__ == "__main__":
pkts = [
Packet(flow=1, idn=1, length=200),
Packet(flow=1, idn=2, length=200),
Packet(flow=10, idn=1, length=200),
Packet(flow=10, idn=2, length=200),
Packet(flow=2, idn=1, length=100),
Packet(flow=2, idn=2, length=100),
Packet(flow=2, idn=3, length=100),
Packet(flow=20, idn=1, length=100),
Packet(flow=20, idn=2, length=100),
Packet(flow=20, idn=3, length=100),
]
Runner(pkts, Hpfq()).run()
24 changes: 13 additions & 11 deletions queue-exp/pifo_srpt.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,15 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
"""

from pifo_lib import Packet, Runner, Pifo
from pifo_lib import SchedulingAlgorithm
from pifo_lib import FlowTracker
from sched_lib import Packet, Runner, Pifo, SchedulingAlgorithm
from sched_lib import FlowTracker


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

def __init__(self):
def __init__(self, name=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

super().__init__(name)
self._pifo = Pifo()
self._flow_tracker = FlowTracker()

Expand All @@ -57,22 +57,24 @@ def __init__(self):
else:
self._remains[pkt.flow] = pkt.length

def get_rank(self, pkt):
rank = self._remains[pkt.flow]
self._remains[pkt.flow] -= pkt.length
def get_rank(self, item):
"""Rank the items by their remaining total flow length."""

rank = self._remains[item.flow]
self._remains[item.flow] -= item.length
return rank

def enqueue(self, item):
def enqueue(self, ref, item):
flow = self._flow_tracker.enqueue(item)
rank = self.get_rank(item)
self._pifo.enqueue(flow, rank)

def dequeue(self):
flow = self._pifo.dequeue()
pkt = None
item = None
if flow is not None:
pkt = flow.dequeue()
return pkt
item = flow.dequeue()
return item

def dump(self):
self._pifo.dump()
Expand Down
19 changes: 11 additions & 8 deletions queue-exp/pifo_stfq.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,34 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
"""

from pifo_lib import Packet, Runner, Pifo
from pifo_lib import SchedulingAlgorithm
from sched_lib import Packet, Runner, Pifo, SchedulingAlgorithm


class Stfq(SchedulingAlgorithm):
"""Start-Time Fair Queuing (STFQ)"""

def __init__(self):
def __init__(self, name=None):
super().__init__(name)
self._pifo = Pifo()

self._last_finish = {}
self._virt_time = 0

def get_rank(self, pkt):
flow_id = pkt.flow
def get_rank(self, item):
"""Rank the items by their start time, which we calculate from the
finish time of the last packet.
"""
flow_id = item.flow
if flow_id in self._last_finish:
rank = max(self._virt_time, self._last_finish[flow_id])
else:
rank = self._virt_time
self._last_finish[flow_id] = rank + pkt.length
self._last_finish[flow_id] = rank + item.length
return rank

def enqueue(self, item):
def enqueue(self, ref, item):
rank = self.get_rank(item)
self._pifo.enqueue(item, rank)
self._pifo.enqueue(ref, rank)

def dequeue(self):
return self._pifo.dequeue()
Expand Down
21 changes: 13 additions & 8 deletions queue-exp/pifo_wfq.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,36 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
"""

from pifo_lib import Packet, Runner, Pifo
from pifo_lib import SchedulingAlgorithm
from sched_lib import Packet, Runner, Pifo, SchedulingAlgorithm


class Wfq(SchedulingAlgorithm):
"""Weighted Fair Queueing (WFQ)"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def __init__(self):
def __init__(self, name=None):
super().__init__(name)
self._pifo = Pifo()
self._last_finish = {}
self._virt_time = 0

def get_rank(self, pkt):
flow = pkt.flow
def get_rank(self, item):
"""Rank the items by their start time, which we calculate from the
finish time of the last packet. However, we divide the packet's length
with weights to prioritize them. We determine the weights by splitting
the flow-ids into odd and even numbers.
"""
flow = item.flow
weight = 50 if flow % 2 == 1 else 100
if flow in self._last_finish:
rank = max(self._virt_time, self._last_finish[flow])
else:
rank = self._virt_time
self._last_finish[flow] = rank + pkt.length / weight
self._last_finish[flow] = rank + item.length / weight
return rank

def enqueue(self, item):
def enqueue(self, ref, item):
rank = self.get_rank(item)
self._pifo.enqueue(item, rank)
self._pifo.enqueue(ref, rank)

def dequeue(self):
return self._pifo.dequeue()
Expand Down
30 changes: 16 additions & 14 deletions queue-exp/pifo_lib.py → queue-exp/sched_lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

print(" Scheduler state:")
self.scheduler.dump()
output = []
Expand All @@ -70,10 +70,10 @@ class SchedulingAlgorithm():
Please look at the pifo_fifo.py to see how you implement a FIFO.
"""

def __init__(self):
raise NotImplementedError(self.__class__.__name__ + ' missing implementation')
def __init__(self, name=None):
self._name = name

def enqueue(self, item):
def enqueue(self, ref, item):
raise NotImplementedError(self.__class__.__name__ + ' missing implementation')

def dequeue(self):
Expand All @@ -83,25 +83,28 @@ def dump(self):
raise NotImplementedError(self.__class__.__name__ + ' missing implementation')

def __next__(self):
item = self.dequeue()
if item is None:
pkt = self.dequeue()
if pkt is None:
raise StopIteration
return item
return pkt

def __iter__(self):
return self

def __repr__(self):
return f"{self.__class__.__name__} - {self.__class__.__doc__}"
result = f"{self.__class__.__name__} - {self.__class__.__doc__}"
if self._name is not None:
result = f"{self._name}: {result}"
return result


class Queue:
def __init__(self, idx=None):
self._list = []
self.idx = idx

def enqueue(self, item):
self._list.append(item)
def enqueue(self, ref, rank=None):
self._list.append(ref)

def peek(self):
try:
Expand Down Expand Up @@ -139,11 +142,11 @@ def dump(self):


class Pifo(Queue):
def enqueue(self, item, rank):
def enqueue(self, ref, rank):
if rank is None:
raise ValueError("Rank can't be of value 'None'.")

super().enqueue((rank, item))
super().enqueue((rank, ref))
self.sort()

def sort(self):
Expand All @@ -160,8 +163,7 @@ def peek(self):

class Flow(Queue):
def __init__(self, idx):
super().__init__()
self.idx = idx
super().__init__(idx)

def __repr__(self):
return f"F(I:{self.idx}, Q:{self.qlen}, L:{self.length})"
Expand Down