<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22343">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22343/2/src/gb/gprs_ns2_frgre.c">File src/gb/gprs_ns2_frgre.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/22343/2/src/gb/gprs_ns2_frgre.c@572">Patch Set #2, Line 572:</a> <code style="font-family:monospace,monospace">1500</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this changes from current behavior.  I think the old code would simply IP-fragment, which is actually intended in that situation.  What matters here is not the MTU of the Ethernet device, but what kind of UDP packet payload size we can transmit.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22343/2/src/gb/gprs_ns2_internal.h">File src/gb/gprs_ns2_internal.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/22343/2/src/gb/gprs_ns2_internal.h@240">Patch Set #2, Line 240:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">is there a reason to use a signed value? like negative having  a special meaning? If yes, please docuemnt in comment, if not I'd suggest unsigned int.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/22343/2/src/gb/gprs_ns2_udp.c">File src/gb/gprs_ns2_udp.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/22343/2/src/gb/gprs_ns2_udp.c@121">Patch Set #2, Line 121:</a> <code style="font-family:monospace,monospace">       int </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">also here I'm not sure if we really should worry about the MTU of the ethernet. </p><p style="white-space: pre-wrap; word-wrap: break-word;">In the end, we have potentially larger BSSGP frames and there is no way to influence the size of the upper layer frames.  Think of a PDP context with MTU 1500 (or close to that), plus the LLC, SNDCP, BSSGP overhead -> boom.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, we may end up generating IP fragments.  But then, the bandwidth of GPRS is ultra low, so what do we care about some more packets in the core network over wired interfaces that likely have more than a thousand time more bandwidth than our radio interface.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If we enforce staying within the MTU of the ethernet device, we would start dropping packets with no way to inform this up the protocol stack so that the LLC/SNDCP XID exchange could negotiate smaller packet sizes. - i.e.  we'd effectively break the network completely.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think the only situation wherer the MTU matters is in the case of FR, where we have a fixed, well-known MTU and no support from the transport (FR) to do segmentation / fragmentation by itself.  Luckily it's 1600, and hence we do have some room for BSSGP/LLC/SNDCP overhead</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/22343">change 22343</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/22343"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I5016b295db6185ec131d83089cf6c806e34ef1b6 </div>
<div style="display:none"> Gerrit-Change-Number: 22343 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 20 Jan 2021 22:02:36 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>